[vqueues] Fix a bug with a lost wakeup on inbox event - #5085
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c86fd62be4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// | ||
| /// Since v1.7.3 | ||
| #[cfg_attr(feature = "schemars", schemars(skip))] | ||
| pub self_proposal_queue_memory_limit: NonZeroByteCount, |
There was a problem hiding this comment.
This adds the user-facing worker.self-proposal-queue-memory-limit option and changes leader self-proposal buffering/backpressure behavior, but the commit does not add any release note under release-notes/. Without a release note, operators upgrading to this version won't see the new per-partition memory limit/default or know how to tune it.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
This PR replaces the current `stream_select` inside `LeaderState::run` with a byte aware DRR scheduler that ensures fairness across the different self proposer writers. A stream (e.g. schema upserts) dumping a 32MB command inside the self proposer, will get penalized for a bit allowing other streams to write to the self proposer. The scheduler refills each stream with 64KB credits per round (an arbitrary number tbh), and fast forwards if all the streams are in deficit. A stream that still has credits will get polled multiple times until it either moves to Pending or exhusts all of its credits. Because we allow the streams to get polled first, then charge back their bytes, a stream can end up with a negative deficit. Streams with negative deficits won't get polled until they're in the positive again. Implementation wise: 1. We're introducing a new scheduler that keeps track of the states of the different flows. This scheduler gets polled for the next flow to be polled, and its returns a decision. The decision struct should then reprot back the result of this poll (bytes written, pending, etc) for the scheduler own tracking. 2. We got rid of the large `LeaderEvents` enum as now no longer need to collect all stream outcomes into one container. 3. One improvement over the current design is that we no longer need to poll all the streams on every `LeaderState::run` call. But that also means that the correctness of wakers are more important as seen in #5085. Perf wise, my local benchmarks report no regression in the simple 1000 connection, single VO gets benchmark: ``` wrk -t1 -c1000 --latency -d30s -s ./scripts/wrk/counter.lua http://localhost:8080 main ✱ ◼ thread 1784720132 started Running 30s test @ http://localhost:8080 1 threads and 1000 connections Thread Stats Avg Stdev Max +/- Stdev Latency 47.07ms 39.32ms 1.10s 99.48% Req/Sec 22.18k 2.73k 32.18k 72.58% Latency Distribution 50% 44.00ms 75% 51.03ms 90% 58.43ms 99% 75.60ms 660389 requests in 30.05s, 175.71MB read Requests/sec: 21979.32 Transfer/sec: 5.85MB thread 1784720132 made 661389 requests and got 660389 responses ```
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
I think originally Ahmed had planned that the scheduler would/could run in its own task and therefore he kept the waker around to wake it up on certain events. With 4f22aa677816#diff-02cc9fd99c15c55730f0822a024e9d2c6bd7f4f92d3cc2d8c9d030773f1d6f6d, we settled on the scheduler being run as part of the partition processor loop. Therefore, if I recall correctly, we removed the waking logic.
If you need this bit for subsequent changes (like the leader event scheduler), then it probably makes sense to revisit this part so that we properly wake the scheduler up on all external signals that transition the scheduler into a state where it should be polled.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010ec45d04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Default: 64 MiB | ||
| /// | ||
| /// Since v1.7.3 | ||
| #[cfg_attr(feature = "schemars", schemars(skip))] |
There was a problem hiding this comment.
This adds the documented worker.self-proposal-queue-memory-limit tuning knob, but schemars(skip) keeps it out of the generated configuration schema (cargo xtask generate-config-schema builds the schema from Configuration). Operators and tooling that rely on that schema for config validation or editor completion will not be able to discover or validate the new backpressure limit, unlike the other public byte-size worker options; unless this is intended to be private, remove the skip.
Useful? React with 👍 / 👎.
AhmedSoliman
left a comment
AhmedSoliman
left a comment
There was a problem hiding this comment.
I'm afraid we will need to have the more elaborate should_wake to reduce spurious wakes on the main pp task.
|
@AhmedSoliman ok, I can port what was there in the old commit 👍 |
The vqueues DRR scheduler wasn't waking up the waker whenever an inbox event happens (e.g. an entry is enqueued). It seems like it was there before, and got removed as part of [this commit](4f22aa677816#diff-02cc9fd99c15c55730f0822a024e9d2c6bd7f4f92d3cc2d8c9d030773f1d6f6d). This is currently not causing any issues because we're constructing a fresh scheduler stream on every `LeaderState::run`, so wakers are not that useful there. However, with the scheduler of the next PR, it relies on the wakers to figure out which streams to poll, and this bug clearly manifests there. For simplicity, I'm callin the waker with every `on_inbox_event` call, though admittedly, some events won't result into different scheduler decisions. The original commit had a more invovled `should_wake` propagation, happy to copy it if deemed necessary. Note: This is a bug that codex found as part of my testing
AhmedSoliman
left a comment
AhmedSoliman
left a comment
There was a problem hiding this comment.
Great work. Thanks for fixing it properly ❤️
3 participants
The vqueues DRR scheduler wasn't waking up the waker whenever an inbox event happens (e.g. an entry is enqueued). It seems like it was there before, and got removed as part of this commit. This is currently not causing any issues because we're constructing a fresh scheduler stream on every
LeaderState::run, so wakers are not that useful there. However, with the scheduler of the next PR, it relies on the wakers to figure out which streams to poll, and this bug clearly manifests there.For simplicity, I'm callin the waker with every
on_inbox_eventcall, though admittedly, some events won't result into different scheduler decisions. The original commit had a more invovledshould_wakepropagation, happy to copy it if deemed necessary.Note: This is a bug that codex found as part of my testing