Repository navigation
redis: order equal-deadline requests by a Redis sequence - #463
Conversation
Requests sharing a deadline second dequeued in random order: scores were whole-second deadlines and Redis orders ties by member bytes, which start with the random request token. Batch callers submit everything with one deadline, so their submission order was lost. The producer stamps EnqueuedAtMs, and InternalRequest.QueueScore scores deadline plus a sub-second fraction: 21 fraction bits over a 2^17 s window before the deadline, 62.5ms steps, exact in float64 for deadlines below 2^32. Enqueue times outside the window clamp to its edges; an unstamped request scores its bare deadline. Release, reclaim and the retry mover recompute the same score, so requeues keep their position. Deadline-proximity counts use an exclusive next-second bound so fractional scores stay in their deadline's bucket. Fixes llm-d#462 Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>
Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The fractional score quantization can still allow equal-deadline requests to dispatch out of enqueue order.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Updates Redis sorted-set ordering so requests with equal deadlines dispatch by enqueue time using fractional queue scores.
Changes:
- Adds enqueue timestamps and fractional queue scoring.
- Preserves scores during release, reclaim, and retry flows.
- Updates backlog bounds and adds unit/e2e coverage plus release notes.
- Review note:
api/internal_api.gohas a moderate issue (3 votes): 62.5 ms quantization can still produce equal scores and unordered dispatch.
| File | Summary |
|---|---|
test/e2e/e2e_test.go |
Adds equal-deadline ordering coverage. |
release-notes.d/unreleased/463.md |
Documents the Redis ordering change. |
producer/redis_sortedset_producer.go |
Stamps requests and applies queue scores. |
producer/redis_sortedset_producer_test.go |
Tests enqueue-based ordering. |
pkg/redis/sortedset_impl.go |
Updates backlog bounds and requeue scoring. |
pkg/redis/sortedset_impl_test.go |
Tests retry and backlog behavior. |
pkg/redis/claim.go |
Preserves scores during claim recovery. |
pkg/redis/claim_test.go |
Verifies reclaimed scores. |
api/internal_api.go |
Adds enqueue metadata and queue-score calculation. |
api/internal_api_test.go |
Tests queue-score behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| step := offset * (1 << queueScoreFractionBits) / queueScoreHorizonMs | ||
| return float64(deadline) + float64(step)/(1<<queueScoreFractionBits) |
There was a problem hiding this comment.
this is an intentional tradeoff of the design. we do lose some precision but I think it's worth not having to depend on field ordering, or making more invasive changes.
There was a problem hiding this comment.
this is likely still intentional, but maybe worth documenting: two submissions sharing a deadline might tie even when they are several minutes/hours/days apart; in particular, enqueue times more than 36h 24m 32s (1<<17) before a shared deadline all clamp to the same score:
// Shared deadline: 2026-09-30 12:00 UTC
queueScore(1790769600, 1790164800000) // A: Sep 23 12:00 UTC
// => 1790769600.0
queueScore(1790769600, 1790510400000) // B: Sep 27 12:00 UTC
// => 1790769600.0
one might expect A to come first since it was submitted earlier
There was a problem hiding this comment.
is 62.5ms precision enough? From the sig batch discussion today, my understanding is that a batch of requests will have same deadline and queue individual requests (as fast as possible).
There was a problem hiding this comment.
it's not high precision enough, I just retested via micro benchmark and got 100ns for a sequential queue in a batch context. Going to change the approach here
Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>
evacchi
left a comment
evacchi
left a comment
There was a problem hiding this comment.
one note, but otherwise seems fine to me
| step := offset * (1 << queueScoreFractionBits) / queueScoreHorizonMs | ||
| return float64(deadline) + float64(step)/(1<<queueScoreFractionBits) |
There was a problem hiding this comment.
this is likely still intentional, but maybe worth documenting: two submissions sharing a deadline might tie even when they are several minutes/hours/days apart; in particular, enqueue times more than 36h 24m 32s (1<<17) before a shared deadline all clamp to the same score:
// Shared deadline: 2026-09-30 12:00 UTC
queueScore(1790769600, 1790164800000) // A: Sep 23 12:00 UTC
// => 1790769600.0
queueScore(1790769600, 1790510400000) // B: Sep 27 12:00 UTC
// => 1790769600.0
one might expect A to come first since it was submitted earlier
The score fraction spans the 36h24m32s before the deadline in 62.5ms steps, so earlier submissions clamp together. Signed-off-by: Will Eaton <weaton@redhat.com>
| queueScoreFractionBits = 21 | ||
| ) | ||
|
|
||
| func queueScore(deadline, enqueuedAtMs int64) float64 { |
There was a problem hiding this comment.
can you add comment for what this function is doing?
Back-to-back submits shared a 62.5ms step and dispatched in random member order (995/1000 out of place). One submit script now INCRs a per-queue, per-deadline counter and splices it into the payload and score. Signed-off-by: Will Eaton <weaton@redhat.com>
wseaton
changed the title
Signed-off-by: Will Eaton <weaton@redhat.com>
Requests sharing a deadline on the redis-sortedset transport now dispatch in exact submission order: equal scores fell back to random member order (995/1000 out of place for a back-to-back batch).
The submit script INCRs a per-queue, per-deadline counter and splices it into the payload (
EnqueueSeq) and score (deadline + seq/2^21) in one round trip; sequential submit cost is within noise.Fixes #462
Release note