feat(spider-scheduler): Add scheduling state of a single resource group for the resource-group-round-robin scheduler core. - #462
Conversation
…p-round-robin scheduler core: * Add `RgSchedulingUnit`, which owns one resource group's job lists, its pending finalizations, and the write side of its dispatch queue, and publishes at most one assignment per turn through `try_make_assignment`. * Add `FinalizeKind` and `MakeAssignmentError` alongside the unit, following the convention that types specific to a scheduling algorithm live inside that algorithm's module. * Export `DOWNGRADE_LIVES` from the job registry to the rest of the module, so that the unit's tests can name the downgrade budget they exhaust. * Restore the dispatch queue registry's `close_dispatch_queue`, `close_broadcast_queue`, and `num_outstanding_hints` as `#[cfg(test)]` accessors. `DispatchQueueRegistryInner` is private to its own module, so the unit's tests cannot otherwise reach the channels a rejected publication is asserted against. * Narrow the job registry's dead-code expectation to non-test builds. The unit's tests are the registry's first consumer, so the expectation is no longer fulfilled in the test build and the plain form fails the lint gate.
* Trim the module docstring to its high-level description. * Drop the variant docstrings of `FinalizeKind` and shorten its own to the state it names. * Shorten `MakeAssignmentError::DispatchQueueClosed`'s docstring to the two queues it can name and the fact that it is fatal. * Move `FinalizeKind` below `MakeAssignmentError`.
…gState`: * Rename the type and move its module from `scheduling_unit.rs` to `scheduling_state.rs`. * Rename the test fixture, its helper, and its field to follow the type. * Update every docstring and comment that named the type, in the job registry and the dispatch queue as well as in the renamed module.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughAdds ChangesResource-group round-robin scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change adds isolated scheduling-state functionality without wiring it into runtime behavior, so no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RgSchedulingState
participant JobRegistry
participant id_issuer
participant RgDispatchQueueWriter
RgSchedulingState->>JobRegistry: select active or pending job
RgSchedulingState->>id_issuer: issue JobId and TaskId
RgSchedulingState->>RgDispatchQueueWriter: publish assignment
RgDispatchQueueWriter-->>RgSchedulingState: report queue-closure error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.97.1)Clippy execution failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| /// Errors returned by assignment decision-making. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] | ||
| pub(super) enum MakeAssignmentError { |
There was a problem hiding this comment.
Can we have a better naming? Maybe AssignmentError or AssignmentDecisionError?
There was a problem hiding this comment.
The naming is to match try_make_assignment method, which is the only exposed API that returns this error.
AssignmentErroris more confusing as it sounds like an error of the assignment.AssignmentDecisionErroris a bit broad while we want to tighten the scope that this error is only used fortry_make_assignment.
Let me improve the docstring for better clarification.
There was a problem hiding this comment.
Updated docstring.
| } | ||
| } | ||
|
|
||
| /// Tops the active job list up to capacity from the pending job queue. |
There was a problem hiding this comment.
| /// Tops the active job list up to capacity from the pending job queue. | |
| /// Fills the active job list up to capacity from the pending job queue. |
There was a problem hiding this comment.
As discussed offline, "top up" is a better term to match the behavior of this method. Rewrite it as "Tops up the active job list to capacity..."
| ) -> Option<(JobId, TaskIndex)> { | ||
| const _: () = assert!( | ||
| 1 == DOWNGRADE_LIVES, | ||
| "An barren active job with no tasks is demoted on its first visit, so this loop never \ |
There was a problem hiding this comment.
| "An barren active job with no tasks is demoted on its first visit, so this loop never \ | |
| "A barren active job with no tasks is demoted on its first visit, so this loop never \ |
There was a problem hiding this comment.
My bad. I replaced "barren" with "exhausted," but I thought the agent switched it back. Will do another round of check to fix it.
| already visited." | ||
| ); | ||
|
|
||
| loop { |
There was a problem hiding this comment.
- We need a comment on this loop. Explain briefly what this loop does.
- It might be my problem, but I don't see any return of
Noneif no active or pending job yields a task. In fact, I don't see any return in this case.
There was a problem hiding this comment.
-
tbh I don't think the implementation of this function is hard to understand if we refer to the implementation doc. I rewrote the loop manually on top of Claude's implementation to make it straightforward:
- Line 275 to 279: the active job list is already empty. We either promote a job from the pending queue or the loop stops.
- Line 281 to 283: Adjust the arm.
- Line 284 to 288: Get the job entry. If the job entry is already deleted by the registry, it should be discarded and replaced by a pending job. This is a well-documented behavior according to the design doc.
- Line 289 to 293: A new task assignment is made.
- Line 295 to 301: The job has no more tasks to schedule. Downgrade it. This is also a documented behavior according to the design doc.
Each section is short and self-explanatory, assuming readers have context of how the scheduling policy works. Documenting the policy details is not the responsibility of an inline comment.
-
In line 276, we apply the try operation to
pop_promotable_job, which returnsNoneif there's no job that can be promoted.Noneis propagated from this call.
2 participants
Description
This is the third piece of the
resource_group_round_robinscheduler core, following the job registry in #444 and the dispatch queue in #449. It landsRgSchedulingState, the per-resource-group state the core draws assignments from. The core that drives it follows in a later PR, so nothing is wired up and no behaviour changes.The existing
round_robincore is untouched.What the module holds
RgSchedulingStateowns one resource group's scheduling position: a queue of owed finalizations, an active job list rotated over by a round-robin arm, a pending job queue that feeds it, and two per-tick downgrade buffers.try_make_assignmentis its whole decision surface — it publishes at most one assignment per call and reports why it could not throughMakeAssignmentError.Finalization tasks are dispatched ahead of regular ones. Regular tasks come from the inner round-robin over active jobs, which is the second level of the core's two-level rotation.
The decisions worth reviewing
A job spends at most one downgrade life per tick. A job that yields nothing loses a life, and is retired once it has none left. The two buffers are what keep that accounting honest: a job removed from the active list and a job passed over in the pending queue are both held aside until
apply_downgradesruns at the end of the tick, so neither can be examined — and charged — twice. Returning them to the pending queue immediately would let a single tick spend the whole budget. Demoted jobs go back to the queue's head with their budget restored; passed-over pending jobs go to its tail with their budget untouched, which is what keeps retirement reachable at all.A publication failure is unrecoverable by design. The task is taken before it is published rather than after. The dispatch queues are unbounded, so
publishcan only fail on a closed queue, and the core answers that by ending the tick and stopping the runtime — there is no later tick to preserve the task for. An earlier revision read the task, published, then removed it; that bought a re-admission that can never happen, at the cost of a three-step dance and a redundant arena lookup per assignment.Admission is bounded by the tick's remaining free space, not by a channel bound.
try_make_assignmentrefuses once the group's queue occupancy reachesfree. This is the dynamic threshold scheme with its control parameter fixed at 1; the queues themselves stay unbounded, as #449 established.pop_regular_task's termination rests onDOWNGRADE_LIVES == 1. With one life, a barren active job is always demoted on its first visit, so the loop can never revisit one and needs no visit counter. That precondition is aconstassert in the function rather than a comment, and it names what a larger budget would require.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Tests