peter-toth commented on code in PR #59794:
URL: https://github.com/apache/spark/pull/59794#discussion_r4380907678
##########
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:
**Finding 1.** After this PR, two more loops log a listener failure without
a guard:
- `invokeListeners` (`TaskContextImpl.scala:434`), for the completion and
failure listeners
- `invokeTaskInterruptListeners` (`TaskContextImpl.scala:330`), for the
interrupt listeners
The description leaves the first one to #59663. But #59663 at
[`05aa3da`](https://github.com/apache/spark/pull/59663/commits/05aa3daf8653a022b1137b001bebd9c868a83f80)
keeps that `logError` unguarded too. It only releases the invocation in a
`finally`. I ran three probes with an appender like the new `FailingAppender`:
- A pre-completion listener and a failure listener fail, and logging the
failure listener's error fails. The completion listener never runs on head.
#59663's `TaskContextImpl` fixes this one, since it releases the invocation.
- The first completion listener fails, and logging it fails. The second
completion listener never runs, and the `AppenderLoggingException` escapes
`markTaskCompleted()`. #59663's `TaskContextImpl` gives the same result.
- The first interrupt listener fails, and logging it fails. The second
interrupt listener never runs, and the `AppenderLoggingException` escapes
`markInterrupted` before the task is marked failed. #59663 does not touch this
loop.
Base behaves like head in all three.
Wrapping the `logError` at `:434` like the one here fixes the first two
probes on head. The remaining completion listener runs, and
`markTaskCompleted()` throws the `TaskCompletionListenerException`:
```scala
listenerExceptions += e
try {
logError(log"Error in ${MDC(LISTENER, name)}", e)
} catch {
case t: Throwable => e.addSuppressed(t)
}
```
The interrupt loop at `:330` takes the same 5 lines. Could you guard both
here, or file a ticket for them? The conflict with #59663 would be the same
kind of rebase as the `FailingAppender`. (The log line in `markInterrupted`
itself is
[r4380697026](https://github.com/apache/spark/pull/59792#discussion_r4380697026)
on #59792.)
--
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]
