HyukjinKwon commented on code in PR #59255:
URL: https://github.com/apache/spark/pull/59255#discussion_r4224408619
##########
common/utils/src/main/scala/org/apache/spark/internal/Logging.scala:
##########
@@ -96,10 +96,10 @@ class LogEntry(messageWithContext: => MessageWithContext) {
def context: java.util.Map[String, String] = cachedMessageWithContext.context
- def +(other: LogEntry): LogEntry = {
- val combined = cachedMessageWithContext + other.cachedMessageWithContext
- new LogEntry(combined)
- }
+ def +(other: LogEntry): LogEntry =
+ // Concatenation passed by name to LogEntry constructor, so that neither
entry is evaluated
+ // unless the result is logged.
+ new LogEntry(cachedMessageWithContext + other.cachedMessageWithContext)
Review Comment:
**Nit (P3):** `SQLLastAttemptAccumulator.logAccumulatorState`
(`sql/core/.../metric/SQLLastAttemptAccumulator.scala:221-234`) wraps
`super.logAccumulatorState + log"""..."""` in a `try` whose `NonFatal` handler
logs a warning and returns `log"<Unexpected exception in
logAccumulatorState>"`. With the old eager `+`, that concatenation ran the base
class's state formatting (`value`, `lastAttemptDirectDriverValue`,
`lastAttemptRddsMap`, ...) inside the `try`, so a formatting failure became the
placeholder. With this change the `try` only builds a thunk. The formatting now
runs later, when `logWarning(logEntry)` in
`LastAttemptAccumulator.unexpectedLastAttemptMetricUpdate` reads the entry,
outside the `catch`.
If that formatting throws, the exception escapes before
`lastAttemptAccumulatorInvalid = true` is set. On the `mergeLastAttempt` path
it ends up in `DAGScheduler.updateAccumulators`' error handler with the
accumulator still marked valid. On the `setValueIfOnDriverSide` path it can
propagate out of the driver-side metric update. The base class's own
`try`/`catch` never covered its interpolation, because the by-name
`LogEntry.from` conversion is applied inside the `try` body, so only the SQL
override loses protection here.
I couldn't find a concrete input that makes the formatting throw, so this
should be rare. Since the guard is deliberate, it may be worth keeping it
effective by moving it into the by-name body, so it runs when the entry is
evaluated and `+` stays lazy. For example:
```scala
override def logAccumulatorState: LogEntry = {
val base = super.logAccumulatorState
LogEntry.from {
try {
val driverQEVals = ...
val entry = base + log"""...""".stripMargin
MessageWithContext(entry.message, entry.context)
} catch {
case NonFatal(e) =>
logWarning(log"Unexpected exception in logAccumulatorState", e)
log"<Unexpected exception in logAccumulatorState>"
}
}
}
```
**Verification:**
- **Regression:** With an SQL last-attempt accumulator whose base state
formatting throws NonFatal, reading the logAccumulatorState entry yields the
placeholder text and does not throw. The merge-target behavior with this PR's
lazy `+` would fail this check.
- **Behavior:** For a well-formed SQL accumulator, the logAccumulatorState
entry still contains the base state and the 'Direct driver QE values' section
once read.
- **Compile:** sql/core main and test sources compile with the reworked
override.
--
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]
