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

[TaskCenter] Requests cancellation of managed runtimes #2364

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Nov 26, 2024

This lets task center request the graceful cancellation of the root-future of managed runtimes. Note that it'll not wait for the shutdown to complete. If this is needed, then partition-processor managed will need to wait for the result to come when it receives the cancellation signal on its own cancellation token.

Stack created with Sapling. Best reviewed with ReviewStack.

This lets task center request the graceful cancellation of the root-future of managed runtimes. Note that it'll not wait for the shutdown to complete. If this is needed, then partition-processor managed will need to wait for the result to come when it receives the cancellation signal on its own cancellation token.
@AhmedSoliman
Copy link
Contributor Author

Partially solves #2341, for the solution to be complete, we need PPM to "await" the oneshot receiver when it receives the cancellation signal.

Copy link

github-actions bot commented Nov 26, 2024

Test Results

  7 files  ±0    7 suites  ±0   4m 20s ⏱️ -7s
 47 tests ±0   46 ✅ ±0  1 💤 ±0  0 ❌ ±0 
182 runs  ±0  179 ✅ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit b88e8b0. ± Comparison against base commit 07882c3.

♻️ 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.

Thanks for fixing the shutdown problem of the partition processors that I've introduced @AhmedSoliman. The changes look good to me. +1 for merging :-)

Comment on lines +643 to +646
if let Some(runtime) = owner {
runtime.shutdown_timeout(Duration::from_secs(2));
trace!("Runtime {} shutdown completed", name);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an expect to encode the invariant that we are the only owner of the runtime at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure as it wouldn't impact correctness of the system. Let's add it when we face a scenario where we need to really be sure that we uphold it.

@AhmedSoliman AhmedSoliman merged commit b88e8b0 into main Nov 27, 2024
11 checks passed
@AhmedSoliman AhmedSoliman deleted the pr2364 branch November 27, 2024 08:44
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