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

Fix rocksdb stall-detector sleep bug #1856

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Fix rocksdb stall-detector sleep bug #1856

merged 3 commits into from
Aug 20, 2024

Conversation

Copy link

github-actions bot commented Aug 16, 2024

Test Results

15 files  ±0  15 suites  ±0   20m 34s ⏱️ -42s
 6 tests ±0   6 ✅ ±0  0 💤 ±0  0 ❌ ±0 
18 runs  ±0  18 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2abb0f9. ± Comparison against base commit c0e373e.

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

LGTM. +1 for merging.

Comment on lines -517 to +518
_ = tokio::time::sleep(manager.stall_detection_duration()), if !stalled => {
_ = &mut timeout, if !stalled => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are trying to save the costs to create one additional tokio::time::sleep::Sleep after it has fired, right? Did you see the costs of this operation somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It showed up as an offender in my profiles, every bit helps :)

Cleanup and simplifying the code a litte, removes a few unnecessary clones. And most importantl, introduces `spawn_unmanaged()` that returns a runtime-agnostic TaskHandle<T>.

Unmanaged tasks:
- Created with an automatic cancellation token, code running in the task would still use the cancellation watcher as per usual
- Tasks that return a value
- Owner of TaskHandle can decide to cancel gracefully or abort.
- Choice of runtime is still encapsulated within TaskCenter

Other changes:
- Important: Scheduling now works differently. Spawned tasks by default "inherit"s the parent's runtime. This reduces remote tokio scheduling and scopes partition processor workloads more accurately to their respective runtimes.
- Direct and fast access to metadata() from `tc.metadata()` is now available.
- Simplified task locals under one struct `TaskContext`
- Moved telemetry submittion within task-center itself (used in subsequent PRs)
- Support background appender
- Configurable base-dir, and other test parameters
- Uses the new unmanaged TC tasks
Fixes an errornous use of sleep in a tokio::select, it overloads the timerwheel, wasting previous CPU cycles.
@AhmedSoliman AhmedSoliman merged commit 2abb0f9 into main Aug 20, 2024
23 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1856 branch August 20, 2024 12:10
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