-
Notifications
You must be signed in to change notification settings - Fork 49
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
Health in TaskCenter #2613
Health in TaskCenter #2613
Conversation
Test Results 7 files ±0 7 suites ±0 3m 40s ⏱️ -48s Results for commit e407716. ± Comparison against base commit d641697. This pull request removes 2 tests.
♻️ This comment has been updated with latest results. |
7ff1daa
to
30a66d5
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.
The changes look good to me. +1 for merging. Note, I will test the whole stack of changes together.
crates/node/src/roles/base.rs
Outdated
// We don't want this task to shutdown by task center, we intentionally want to leak it and | ||
// leave it running until the runtime is terminated. | ||
// | ||
// This is because we want to try and response with NodeStatus::ShuttingDown for as long as |
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.
// This is because we want to try and response with NodeStatus::ShuttingDown for as long as | |
// This is because we want to respond while NodeStatus::ShuttingDown for as long as |
?
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.
This actually is old comment. will remove.
/// Cluster controller is the first thing that gets stopped when the server is shut down | ||
ClusterController, |
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.
How is this enforced?
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.
in the next PR.
self.inner | ||
.health | ||
.node_status() | ||
.merge(NodeStatus::StartingUp); |
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.
The NodeStatus
is spread across the code base with this change. But I guess w/o structured concurrency there is no way to keep it in fewer places.
a8bbd8a
to
a69c11a
Compare
e6faa60
to
9876c9b
Compare
Stack created with Sapling. Best reviewed with ReviewStack.