You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "juliuszsompolski (via GitHub)" <gi...@apache.org> on 2026/10/06 10:24:38 UTC
[PR] [SPARK-59998][CORE] Make LogEntry concatenation lazy [spark]
juliuszsompolski opened a new pull request, #59255:
URL: https://github.com/apache/spark/pull/59255
### What changes were proposed in this pull request?
`LogEntry.+` bound the concatenation to a `val`, which evaluated both entries as soon as `+` was called. This PR passes the concatenation to the by-name `LogEntry` constructor instead, so it is only evaluated when the entry is logged.
### Why are the changes needed?
`LogEntry` is lazy so that a message is only formatted if its log level is enabled, but `+` (added in SPARK-56509) formatted both sides eagerly: `logTrace(log"..." + entry)` evaluated everything even with TRACE disabled. For example, `LastAttemptAccumulator.mergeLastAttempt` formats the accumulator's per-partition state into TRACE messages on every task completion.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added a test to `LoggingSuiteBase` (run by `StructuredLoggingSuite` and `PatternLoggingSuite`) that logs a concatenated entry with `logTrace` at INFO, DEBUG and TRACE, and checks that both sides of `+` are evaluated only at TRACE. It fails without this change.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5-5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59998][CORE] Make LogEntry concatenation lazy [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59998][CORE] Make LogEntry concatenation lazy [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #59255: [SPARK-59998][CORE] Make LogEntry concatenation lazy
URL: https://github.com/apache/spark/pull/59255
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59998][CORE] Make LogEntry concatenation lazy [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #59255:
URL: https://github.com/apache/spark/pull/59255#issuecomment-6014281062
cc @cloud-fan @HyukjinKwon this originally landed in Spark 4.2 with [SPARK-56509](https://issues.apache.org/jira/browse/SPARK-56509), so I would recommend backporting down to branch-4.2, since this is the kind of small regression that can get you.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59998][CORE] Make LogEntry concatenation lazy [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #59255:
URL: https://github.com/apache/spark/pull/59255#issuecomment-6133274962
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/c79c1a3f8b9e34bcb99a8b857a126d1b1e003a7c
- merged into branch-4.x https://github.com/apache/spark/commit/c4687ca35c0fcdbab5cc0ebe22100fe34351e1c0
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59998][CORE] Make LogEntry concatenation lazy [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #59255:
URL: https://github.com/apache/spark/pull/59255#issuecomment-6082436956
@HyukjinKwon good catch! Fixed. Could you take another look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org