dongjoon-hyun commented on code in PR #59794:
URL: https://github.com/apache/spark/pull/59794#discussion_r4381089230
##########
core/src/main/scala/org/apache/spark/TaskContextImpl.scala:
##########
@@ -268,7 +268,14 @@ private[spark] class TaskContextImpl(
case t: Throwable => e.addSuppressed(t)
}
listenerExceptions += e
- logError(log"Error in ${MDC(LISTENER, "TaskPreCompletionListener")}",
e)
+ // Best effort: a failure to log, e.g. from an appender that does not
ignore exceptions,
+ // must not end the loop in `markTaskCompleted()` before it sets
`completed`, or the
+ // completion listeners would never run. It is reported with `e`
instead.
+ try {
+ logError(log"Error in ${MDC(LISTENER,
"TaskPreCompletionListener")}", e)
Review Comment:
Thanks for the review, @peter-toth. I reproduced the three probes on
f10a5ce9aa4.
I guarded `invokeListeners()` here with the same five lines in 6027637d425,
and added two tests: one for a failing completion listener (your second probe)
and one for a failing failure listener of a failed pre-completion listener
(your first probe). Both fail without the guard. The first probe is reachable
through the path this PR fixes, since `markTaskFailed(e)` runs the failure
listeners, so it belongs here.
I would keep the interrupt loop out of this PR.
`invokeTaskInterruptListeners()` is on the kill path, and apache/spark#59792
was merged guarding only `markTaskFailed(e)` in `markInterrupted`: its
`logError` and the one in the loop are still unguarded. I will handle both in a
separate change.
--
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]
