HyukjinKwon commented on code in PR #59229:
URL: https://github.com/apache/spark/pull/59229#discussion_r4175787943
##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -950,12 +968,10 @@ private[spark] class ExecutorAllocationManager(
// (SPARK-8366)
allocationManager.onSchedulerBacklogged()
}
- if (!taskEnd.taskInfo.speculative) {
- // If a non-speculative task is intentionally killed, it means
the speculative task
- // has succeeded, and no further task of this task index will be
resubmitted. In this
- // case, the task index is completed and we shouldn't remove it
from
- // stageAttemptToTaskIndices. Otherwise, we will have a pending
non-speculative task
- // for the task index (SPARK-30511)
+ if (!taskEnd.taskInfo.speculative &&
Review Comment:
**Nit (P3):** This new guard -- skip the index removal when the speculative
copy already succeeded -- is only reached on a non-killed failure, and no test
drives that path. All four added tests post `TaskKilled` (the sibling arm), and
the existing `SPARK-30511` test fails index 38 *before* its speculative copy
succeeds, so it removes the index exactly as the old code did and never crosses
this guard. A regression that dropped the guard would re-pend an
already-completed index -- raising `maxNumExecutorsNeededPerResourceProfile` --
with every test still green. Worth adding a case where a regular task's
speculative copy succeeds and the regular attempt then ends with a non-killed
failure (e.g. `ExceptionFailure`/`UnknownReason`), asserting the index stays
counted as done.
**Verification:**
- **Regression:** A test exercising the case _ => arm with a non-killed
failure after a recorded speculative success fails when the
successful-speculative guard is removed (index re-pended, maxNumExecutorsNeeded
rises) and passes with the guard present.
--
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]
