-
Notifications
You must be signed in to change notification settings - Fork 44
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
Attach to cluster controller through Networking #1500
Conversation
Test Results 99 files ±0 99 suites ±0 8m 5s ⏱️ -51s Results for commit 4f22a93. ± Comparison against base commit 1d7ac91. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Linter error is on generated code, this will be fixed in the next PR in this stack. |
Significant changes are incoming to this PR, putting this back as WIP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some intermediate review comments for a718fcd. Waiting for the PR to be updated before continuing with the review.
baeeec4
to
25896ae
Compare
This should be ready for review. PTAL @tillrohrmann. |
2a5c6d9
to
bce7fb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @AhmedSoliman. LGTM. +1 for merging :-)
Ok(Response::new(AttachmentResponse {})) | ||
_request: tonic::Request<ClusterStateRequest>, | ||
) -> Result<tonic::Response<ClusterStateResponse>, tonic::Status> { | ||
unimplemented!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that anyone is calling this endpoint but maybe let's return an error instead of causing a panic in case a stranger calls it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't panic the runtime, but better to change this to an error code instead.
running_partition_processors: HashMap::default(), | ||
running_partition_processors: BTreeMap::default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is BTreeMap
faster than HashMap
for small number of entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it's also more cacheline friendly, and in general better when keys are small and numeric.
This PR includes: - Attachment of PPM to cluster controller now use Networking - PPM observes partition processor status through a buffered watch mechanism - PPM can now send control messages (unused at this PR) to processor for future use. - PPM collects state information from running processors. This will be used in a follow PR to response to controller requests about partitions. Cluster controller grpc service is kept for external tooling integration (CLI, etc.)
Attach to cluster controller through Networking
This PR includes:
Cluster controller grpc service is kept for external tooling integration (CLI, etc.)
Stack created with Sapling. Best reviewed with ReviewStack.