Check for eligibility on every removed item #4070
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AhmedSoliman
reviewed
Dec 1, 2025
crates/vqueues/src/scheduler/drr.rs
Outdated
Comment on lines
382
to
387
| qstate.notify_removed(item_hash); | ||
|
|
||
| // if we aren't eligible check whether the removed item has made us eligible | ||
| // again (e.g. by releasing a concurrency token or by removing the head of the | ||
| // queue). | ||
| if !self.eligible.contains(&qid) { |
Contributor
There was a problem hiding this comment.
This seems to overlap with the change in https://github.com/restatedev/restate/pull/4066/files#diff-02cc9fd99c15c55730f0822a024e9d2c6bd7f4f92d3cc2d8c9d030773f1d6f6d
Contributor
Author
There was a problem hiding this comment.
Yes, you are right. Rebasing this PR on #4066.
This commit integrates vqueues with external state mutations. The way it works is by storing the external state mutation in the VQueueItems table until it gets run by the scheduler. When running the state mutation, we retrieve it from the VQueueItems table and apply the state mutation changes. The external state mutation EntryId is currently derived from the internal inbox_seq_number to generate unique EntryId's. In the future we might give external state mutations an explicit id which could be used to derive the EntryId. Currently, the DRR scheduler reserves a concurrency token for running state mutations. We might want to relax this condition if we see that applying state mutations does not need to be protected by the global concurrency limit.
Introduces a small set of cumulative metrics from the vqueue scheduler to help visualize: - The rate of items in scheduler decisions broken down by action - The enqueue rate (in waiting inbox) - Counters for confirmed/rejected items (confirmed items is low signal and we can remove it)
Before, the DRR scheduler checked for eligibility if the current head item was removed. This was not enough since a vqueue might have been not eligible because it was running out of concurrency tokens. A removed item could have released the concurrency token and thereby it could make the vqueue eligible again if the vqueue contains more items. Hence, we should check on every removed item whether this event had an impact on the vqueue's eligibility.
tillrohrmann
force-pushed
the
vqueues/fix-eligibility
branch
from
3edf6ed to
4219335
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before, the DRR scheduler checked for eligibility if the current head item
was removed. This was not enough since a vqueue might have been not eligible
because it was running out of concurrency tokens. A removed item could have
released the concurrency token and thereby it could make the vqueue eligible
again if the vqueue contains more items. Hence, we should check on every
removed item whether this event had an impact on the vqueue's eligibility.
This PR is based on #4059.