dongjoon-hyun commented on code in PR #59663:
URL: https://github.com/apache/spark/pull/59663#discussion_r4339138551
##########
core/src/main/scala/org/apache/spark/TaskContextImpl.scala:
##########
@@ -217,12 +239,45 @@ private[spark] class TaskContextImpl(
// writer that adds listeners lazily while it pulls its input. And since
the completion
// listeners only run after `completed` is set, they run after every
pre-completion listener,
// even if another thread is invoking listeners at this point.
+ //
+ // The completion listeners then run on this thread too, and this method
returns only after
+ // they have run. If it returned while another thread is invoking
listeners (e.g. a kill thread
+ // running failure listeners), that thread would not run them, so cleanup
would be skipped,
Review Comment:
**8. [Low] Several comments still argue from the skipped-listener behavior**
"that thread would not run them, so cleanup would be skipped" describes
master. With the leftover branch in `nextStep()` (L595-597), a holder runs the
completion listeners left to it when nobody waits, as L250-252 and L353-355
say. What the wait prevents is that they run late, on that thread, after the
task has released its memory and reported its result.
The same goes for comments that this PR does not touch:
- L446-449: sharing `invokeListeners()` "would prevent task completion or
failure listeners from running"; now `markTaskCompleted` would wait for the
interrupt listeners instead.
- `StreamingShuffleSuite.scala:1447-1448` still says "With the
CompletableFuture approach, ... (no race with the completion listener)", while
the rewritten test above it now says `ErrorNotifier`.
- `StreamingShuffleSuite.scala:1486-1488`, `1497-1502` and `1559-1563` say
there is "no contention on the listenerInvocationThread guard", so the
completion listener runs normally.
Could they be updated together with the `ErrorNotifier` and
`StreamingShuffleReader` comments? Here, e.g.: "..., that thread would run them
only after its own listeners, after the task has released its memory and
reported its result."
--
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]
