Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return Node instances from GetNodes #1936

Merged
merged 10 commits into from
Sep 12, 2023
Merged

Return Node instances from GetNodes #1936

merged 10 commits into from
Sep 12, 2023

Conversation

danieljanes
Copy link
Member

Issue

Description

The Driver API returns individual int node IDs, not full Node instances. When scheduling a Task, one has to construct a full Node instance (setting the node_id and the anonymous flag). Changes to the Node message are not reflected in GetNodes and it is unclear how additional Node properties would be communicated to the driver script.

Related issues/PRs

None

Proposal

Explanation

If GetNodes returned full Node instances, one could just sample from that list and set one of the Node instances as the consumer of the new Task. Changes to the Node message (e.g., additional properties) would automatically be reflected in GetNodes.

This PR returns full Node instances instead of int node IDs from GetNodes.

Checklist

  • Implement proposed change
  • Write tests
  • Update documentation
  • Update changelog
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

@tanertopal
Copy link
Member

@danieljanes, my thoughts on this:

  • Changing from int64 to a sub-message will increase the response size, making it less scalable.
  • The Driver potentially has to pull all node ids, e.g. 10M+, which was our initial thinking for choosing int64.
  • If we need a more verbose response, how about splitting it into two methods?

@tanertopal tanertopal enabled auto-merge (squash) September 12, 2023 10:25
@tanertopal tanertopal merged commit 3c86e8a into main Sep 12, 2023
26 checks passed
@tanertopal tanertopal deleted the return-nodes branch September 12, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants