HyukjinKwon commented on code in PR #59229:
URL: https://github.com/apache/spark/pull/59229#discussion_r4227649704
##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -950,12 +986,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 &&
+
!stageAttemptToSuccessfulSpeculativeTaskIndices.get(stageAttempt)
+ .exists(_.contains(taskIndex))) {
+ // An unsuccessful regular task remains pending until another
attempt starts.
Review Comment:
**Nit (P3):** This comment doesn't match how the listener ends the pending
state. The index goes back into `stageAttemptToTaskIndices` in two places:
- `onTaskStart`, but only for a regular attempt.
- the `Success` arm above (lines 953-954), for any successful attempt.
So a speculative copy that was already running ends the pending state when
it succeeds, without any new attempt starting. The `stop counting a task as
pending when speculation succeeds after $reason` tests rely on exactly that. A
speculative attempt starting, on the other hand, does not end it. This
accounting has been easy to get wrong across revisions, so the comment should
name the real exits:
```suggestion
// The index stays pending until a regular attempt starts or
any attempt succeeds.
```
**Verification:**
- **Inspection:** The revised comment names both exits implemented by the
listener (a regular attempt start in onTaskStart and any successful attempt in
the Success arm) and does not claim that a speculative attempt start ends the
pending state.
- **Compile:** The core module still compiles and passes style checks, with
the edited line within the 100-character limit and ASCII only.
--
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]
