zahed1994 commented on PR #58380:
URL: https://github.com/apache/spark/pull/58380#issuecomment-5475191035
Thanks @sunchao, good catch on the StatefulSet case.
You're right that the previous logic could cause the target to drop back to
2 once the additional executor was registered or the speculative task moved
from pending to running. In the StatefulSet allocator, that can immediately
trigger a scale-down and remove the newly allocated executor before the
speculative task has a chance to make progress.
I've updated the allocation logic in `ExecutorAllocationManager.scala` to
account for the full lifecycle of the speculative task rather than only the
pending state.
Specifically:
1. **Track the full speculative task lifecycle**
`ExecutorAllocationListener` now provides the number of speculative tasks
that are either pending or currently running. The allocation logic uses:
`speculativeTasks = pendingSpeculative + runningSpeculative`
This ensures that the additional executor remains accounted for after the
speculative task has started running.
2. **Use the regular-task requirement as the baseline**
Instead of comparing `maxNeeded` against the current active executor
count, I calculate `baseMaxNeededWithoutSpeculation` from the regular
pending/running tasks (excluding running speculative tasks).
The locality offset is then applied when:
`maxNeeded == baseMaxNeededWithoutSpeculation`
and there is an active speculative task. This keeps the additional
executor tied to the actual resource requirement of the regular workload,
rather than the transient number of executors that happen to be registered at
that point.
3. **Retain the additional executor while speculation is running**
With this change, the allocation remains stable across the speculative
task transition:
`pending → running`
So in the case you described, the target remains at 3 after executor #3
registers and after the speculative task starts, instead of immediately
returning to 2 and allowing the StatefulSet allocator to remove the additional
executor.
Once the speculative attempt finishes, the speculative-task count goes
back to zero and the target can return to the regular-task requirement of 2.
4. **Handle the killed-speculation path**
I also added coverage for the case where the speculative attempt is
killed rather than completing normally. The additional executor is retained
while the speculative task is running and released once the speculative attempt
ends.
I've added regression coverage in `ExecutorAllocationManagerSuite` for both:
`pending → running → completed`
and
`pending → running → killed`
The targeted tests and linters are passing cleanly.
Could you please take another look when you get a chance?
--
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]
