fix: Fairly allocate partitions from scheduler assignment cycle only#11853
fix: Fairly allocate partitions from scheduler assignment cycle only#11853peasee wants to merge 9 commits into
Conversation
✅ Pull with Spice PassedPassing checks:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors cluster partition assignment so initial partition ownership is determined by the scheduler’s periodic fair-share assignment cycle (instead of executors greedily allocating on connect), and makes executors retry allocate_initial_partitions until the scheduler is ready to return a fair initial share.
Changes:
- Delay the first scheduler partition-assignment cycle by one interval and introduce a “first assignment complete” gate to control when executors can fetch initial assignments.
- Change
allocate_initial_partitionsto return only partitions already assigned to the requesting executor (no longer mutating assignments). - Add an integration test assertion to validate fair-share distribution across multiple executors at startup.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/runtime/tests/cluster/harness.rs | Adds helper to read per-executor partition ownership from scheduler metadata for assertions. |
| crates/runtime/tests/cluster/distributed_acceleration.rs | Adds a test assertion that initial partitions are fairly split across two executors. |
| crates/runtime/src/cluster/service.rs | Updates allocate_initial_partitions to gate on first assignment and stop performing allocation. |
| crates/runtime/src/cluster/partition/scheduler_task.rs | Delays first assignment tick; marks the “first assignment complete” gate after a successful cycle. |
| crates/runtime-cluster/src/store.rs | Removes executor-driven allocate_partitions API and the associated AllocationResult type. |
| crates/runtime-cluster/src/service.rs | Adds first_assignment_complete gate state to the partition service. |
| crates/runtime-cluster/src/lib.rs | Removes re-export of AllocationResult after its deletion. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/runtime/src/cluster/partition/scheduler_task.rs:124
- The first assignment cycle is deferred by
self.interval, but executors only retryallocate_initial_partitionsfor a bounded time onUnavailable(seeALLOCATE_INITIAL_PARTITIONS_MAX_RETRIES+SCHEDULER_BACKOFF_MAXincrates/runtime/src/cluster/mod.rs:86-92, ~5 minutes worst-case). Ifpartition_assignment_intervalis configured larger than that, executors can hard-fail startup before the scheduler ever runs its first cycle and opens the gate.
delay_ms = self.interval.as_millis(),
"Deferring first partition assignment cycle to let executors connect"
);
let start = tokio::time::Instant::now() + self.interval;
let mut interval = tokio::time::interval_at(start, self.interval);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/runtime/src/cluster/partition/scheduler_task.rs:123
- The first partition-assignment cycle is deferred by a full
partition_assignment_interval. Since executors retryallocate_initial_partitionsfor a bounded time (~5 minutes total), configuring a long interval (e.g. >5m) will reliably prevent executors from starting because the scheduler won't assign anything before the retry budget is exhausted. Consider using a small bounded startup window (or a dedicated config field) for the initial delay instead of coupling it to the steady-state interval.
tracing::info!(
delay_ms = self.interval.as_millis(),
"Deferring first partition assignment cycle to let executors connect"
);
let start = tokio::time::Instant::now() + self.interval;
crates/runtime/src/cluster/service.rs:699
- The first-assignment gate blocks all
allocate_initial_partitionscalls until the scheduler runs an assignment pass. On scheduler restart, the partition store may already contain assignments for an executor, but this gate will still force the executor to retry (and potentially time out) even though the scheduler could safely return the already-assigned partitions immediately. Consider bypassing the gate when the cached partition metadata shows this executor already owns at least one partition.
if let Some(partition_service) = self.datafusion.partition_service.as_ref()
&& !partition_service.is_first_assignment_complete()
&& self.has_partitioned_accelerated_tables().await
{
tracing::debug!(
phillipleblanc
left a comment
There was a problem hiding this comment.
I think the correct solution would be to have a proper rebalancing control loop that doesn't depend on executors coming in right after startup, but this at least is an incrementally better solution than what we already had.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/runtime/src/cluster/partition/scheduler_task.rs:116
- Deferring the first partition-assignment cycle by a full
self.intervalmeans executors will receiveUnavailablefromallocate_initial_partitionsfor at least one whole interval (and potentially nearly two intervals if they connect just after the first tick andReconcileOutcome::NoAssignmentkeeps the gate closed).
With long partition_assignment_interval values this can cause executor startup to fail after exhausting its Unavailable retry budget (see crates/runtime/src/cluster/mod.rs: SCHEDULER_BACKOFF_MAX + ALLOCATE_INITIAL_PARTITIONS_MAX_RETRIES). Consider decoupling the startup grace period from the steady-state assignment interval (e.g., separate config, or a bounded grace window) so “fair startup” doesn’t become “minutes-long startup stall”.
// Defer the *first* assignment cycle by one full interval so executors
// have a window to connect before the scheduler distributes partitions.
// Assignment is scheduler-controlled (executors no longer allocate on
// connect via `allocate_initial_partitions`), so an executor that joins
// late — but within this window — is included in the initial fair
| // Gate on the scheduler's first assignment cycle. Returning already-assigned | ||
| // partitions before the scheduler has fairly distributed them would hand this | ||
| // executor an empty set, and its initial snapshot would load zero rows with no | ||
| // way to backfill (CDC/Changes-mode accelerations only load partition data at | ||
| // the initial snapshot). Wait for the first cycle — the executor retries this | ||
| // RPC on `Unavailable` with backoff — so the returned share is the fair one. | ||
| // | ||
| // Only gate when there are accelerated partitioned tables to assign. With | ||
| // none, the scheduler never assigns anything, so blocking would just make | ||
| // executors retry for a full assignment interval during startup for no | ||
| // reason. | ||
| if let Some(partition_service) = self.datafusion.partition_service.as_ref() | ||
| && !partition_service.is_first_assignment_complete() | ||
| && self.has_partitioned_accelerated_tables().await | ||
| { |
📝 Summary
Unavailableto executors requesting their initial partition allocations until the first allocation assignment interval passes. This ensures that executors have some time to connect - instead of the first executor racing to greedily allocate all of the partitions for a table to itself👀 Notes for Reviewers
Unavailableto the executors forces them to re-check their initial assignments in a retry loop, but if the partition assignment interval is too long that retry loop will fail before the scheduler allocates any initial partitions. I think partition assignment, including initial partitions, needs to be entirely push-based instead of executors pulling partitions on first startup - but that requires some additional work.