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

Health in TaskCenter #2613

Closed
wants to merge 1 commit into from
Closed

Health in TaskCenter #2613

wants to merge 1 commit into from

Conversation

Copy link

github-actions bot commented Feb 3, 2025

Test Results

  7 files  ±0    7 suites  ±0   3m 40s ⏱️ -48s
 45 tests  - 2   44 ✅  - 2  1 💤 ±0  0 ❌ ±0 
174 runs   - 8  171 ✅  - 8  3 💤 ±0  0 ❌ ±0 

Results for commit e407716. ± Comparison against base commit d641697.

This pull request removes 2 tests.
dev.restate.sdktesting.tests.AwaitTimeout ‑ timeout(Client)
dev.restate.sdktesting.tests.RawHandler ‑ rawHandler(Client)

♻️ This comment has been updated with latest results.

Copy link
Contributor

@tillrohrmann tillrohrmann left a 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.

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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

?

Copy link
Contributor Author

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.

Comment on lines +96 to +97
/// Cluster controller is the first thing that gets stopped when the server is shut down
ClusterController,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this enforced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the next PR.

Comment on lines +62 to +65
self.inner
.health
.node_status()
.merge(NodeStatus::StartingUp);
Copy link
Contributor

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.

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