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 `invokeTaskInterruptListeners()` out of this PR. It is on the
kill path that apache/spark#59792 changes, so I plan to guard its `logError`
there, next to the one in `markInterrupted` that you pointed out.
--
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]
