-
Notifications
You must be signed in to change notification settings - Fork 565
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 timer scheduling #10534
Fix timer scheduling #10534
Conversation
If we already scheduled a Task for a created Timer, we shouldn't schedule another Task on next timer creation. This commit fixes that. The previous behavior could severe issues, like exponential increasing backlog if a lot of timers are created per second etc.
I have run some benchmarks to compare the fix. I was not able yet to run the E2E tests, but I will do with a next release candidate. BenchmarksBenchmarks only timers (rate 100 PI/s) Timer Benchmark with bugTimer Benchmark with fixWe can see that it is much more stable in the processing and fewer events are skipped. The backpressure is reduced by half. Mixed Benchmark with bugFor that I used the medic cw 39 (and checked the begin of the benchmark). The begin of the mixed looks always okayisch later it shows higher backpressure. But we can see in the begin already issues with the latencies. Mixed Benchmark with fixIn the fixed mixed benchmark we see in general processing not much difference, but in the latency which looks much stable. I will run this benchmark for a while. After ca 5 hours the mixed often failed, so lets see. But I think we can already merge 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.
@Zelldon looks good. 👍
bors merge |
10534: Fix timer scheduling r=saig0 a=Zelldon ## Description If we already scheduled a Task for a created Timer, we shouldn't schedule another Task on next timer creation. This PR fixes that. The previous behavior could cause severe issues, like exponential increasing backlog if a lot of timers are created per second etc. see #10532 and #10526 <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10532 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Build failed: |
bors r+ |
10534: Fix timer scheduling r=Zelldon a=Zelldon ## Description If we already scheduled a Task for a created Timer, we shouldn't schedule another Task on next timer creation. This PR fixes that. The previous behavior could cause severe issues, like exponential increasing backlog if a lot of timers are created per second etc. see #10532 and #10526 <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10532 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
Build failed: |
bors r+ NOW |
Build succeeded: |
Successfully created backport PR #10535 for |
Description
If we already scheduled a Task for a created Timer, we shouldn't schedule another Task on next timer creation. This PR fixes that.
The previous behavior could cause severe issues, like exponential increasing backlog if a lot of timers are created per second etc. see #10532 and #10526
Related issues
closes #10532
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.