Repository navigation
Conversation
Bound tasks are recorded as Running at bind time. When launch_tasks cannot get executor metadata, apache#2689 refunds the vcore reservations but left those tasks stranded as Running with no status updates, so the job hung until the executor was lost. Reset each unlaunched task back to pending (as pop_next_task's contract requires) before refunding vcores, and assert pending work is restored in the metadata-miss test. Closes apache#2691.
| /// `Running` with no status updates ever arriving. See #2691. | ||
| /// | ||
| /// Returns `true` when the task was still `Running` and was reset. | ||
| fn reset_unlaunched_task(&mut self, key: &TaskKey) -> bool; |
There was a problem hiding this comment.
Since ExecutionGraph is a public trait, adding a required method breaks anyone implementing it outside the repo. Could we give it a default body that returns false? That matches the "nothing was reset" meaning in the doc, and both built-in graphs override it anyway, so behaviour here doesn't change. Otherwise it'll need the api-change label and an upgrade-guide note.
There was a problem hiding this comment.
Done in cdf608d: reset_unlaunched_task now has a default body that returns false, so out-of-tree ExecutionGraph implementations keep compiling. StaticExecutionGraph and AdaptiveExecutionGraph both override it. Sorry for not replying here earlier.
I also merged current main in 9b14201 to clear the conflict in state/aqe/mod.rs (an import list). cargo clippy -p ballista-scheduler --all-targets -D warnings is clean, and cargo test -p ballista-scheduler --lib passes (627 passed, 2 ignored).
(AI-assisted reply.)
ExecutionGraph is a public trait; a required method would break out-of-tree implementations. Default to resetting nothing. Signed-off-by: Vincent <[email protected]>
Resolve the trait through the parent module so moving the plan-rewrite test helper and its imports does not break the merged test build. AI-assisted. Static name-resolution and focused diff checks performed; Rust compilation and rustfmt were not run locally (toolchain unavailable). The merged build is left to GitHub Actions.
…unched-on-metadata-miss Resolve the aqe/mod.rs import conflict (keep TaskKey alongside main's ExecutorEndpoint) and drop the super::ExecutionPlan qualification in the cache reader test helper, which main's test-module imports make unnecessary.
Which issue does this PR close?
Closes #2691.
Rationale for this change
#2689 refunds vcore reservations when
launch_taskscannot get executor metadata, but the tasks in that batch were already recorded asRunningat bind time. They were never launched, never reported, and never reset, so the job hung until the executor was lost. Because the refund setsif_revive, a repeated miss also binds more partitions onto the same stranded path.pop_next_task's contract already says a bound-but-unlaunched task must have its status reset so it can be scheduled elsewhere; this fills that gap for the metadata-miss branch.What changes are included in this PR?
ExecutionGraph::reset_unlaunched_task(static + AQE) that returns a still-Runningtask_info slot to pending viareset_task_info.SchedulerState::launch_tasks, callTaskManager::reset_unlaunched_tasksbefore refunding reservations.launch_tasks_refunds_vcores_when_executor_metadata_missingto assert pending task count is restored after the miss.Are there any user-facing changes?
No.
This PR was prepared with AI assistance; I reviewed the change and ran the checks above.