andygrove opened a new pull request, #2240:
URL: https://github.com/apache/datafusion-ballista/pull/2240
# Which issue does this PR close?
Closes #2226.
# Rationale for this change
`killing_every_executor_terminates_the_job::case_2_aqe_on` has been failing
intermittently on CI: the query never terminates and the test's 120s guard
fires. The grace-period fix for #2029 (#2212) did not cover it.
The scheduler discovers that an executor is gone in two different places, and
only one of them arms the empty-cluster grace timer:
- The heartbeat reaper calls `SchedulerServer::remove_executor`, which posts
`QueryStageSchedulerEvent::ExecutorLost`. The handler for that event rolls
back the affected execution graphs, cancels their running tasks and, if no
executors remain, arms the timer that fails the jobs left behind.
- A failing task launch calls `SchedulerState::remove_executor`, which did
the
rollback and cancellation inline and posted no event, so the timer was
never
armed.
The second path also deletes the executor's heartbeat, so the reaper can
never
rediscover the executor and post the event later. When a whole cluster dies
while tasks are being launched onto it, no executor-loss event ever reaches
the
handler: the job stays `Running` forever while the client polls its status
every 50ms. Whether a kill is noticed by a failing launch or by heartbeat
expiry is a timing race, which is why the scenario failed only sometimes and
only under some plans.
The scheduler log from a reproduced hang shows the whole story: both
executors
removed with reason `Failed to launch new task: ... broken pipe`, both stages
reset, then nothing. No `heartbeat timed out`, because there was no heartbeat
left to expire.
# What changes are included in this PR?
- `SchedulerState::remove_executor` now posts `ExecutorLost` instead of
duplicating the loss handling, so both removal paths converge on the single
handler in `query_stage_scheduler.rs`. The event sender is threaded in from
`revive_offers` through `launch_tasks`. Ordering is unchanged: the event is
posted after the executor is removed from `ExecutorManager` and before the
`ReviveOffers` that follows the launch batch.
- New unit test
`test_running_job_fails_when_launch_failure_loses_last_executor`
covering the launch-failure path deterministically, plus a
`lose_executor_on_launch_failure` harness helper alongside the existing
`lose_executor`, which mirrors the reaper. The new test fails on `main`
(the job is still `Running` when the assertion runs) and passes here; the
existing reaper-path test passes either way, which is why this gap went
unnoticed.
- A note in `chaos-testing/README.md` under Finding 3 recording the second
path.
Verification: `cargo test -p ballista-scheduler` (325 passed), the full
`cargo test -p ballista-chaos --test ha -- --test-threads=1` suite (10
passed,
6 pre-existing ignores), `cargo fmt --all -- --check`, `cargo clippy
-p ballista-scheduler --all-targets --all-features -- -D warnings`, and
`cargo check -p ballista-scheduler --no-default-features`.
# Are there any user-facing changes?
No API changes. A job whose executors are all lost during task launch now
fails
with the same "all executors were lost ..." error that the heartbeat-expiry
path already produced, instead of hanging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
