HeartSaVioR commented on code in PR #47794:
URL: https://github.com/apache/spark/pull/47794#discussion_r1721895105
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -1286,12 +1286,12 @@ private[history] class FsHistoryProvider(conf:
SparkConf, clock: Clock)
rebuildAppStore(store, reader, attempt.info.lastUpdated.getTime())
hybridStore = store
} catch {
- case _: IOException if !retried =>
+ case ioe: IOException if !retried =>
// compaction may touch the file(s) which app rebuild wants to read
// compaction wouldn't run in short interval, so try again...
logWarning(log"Exception occurred while rebuilding log path " +
log"${MDC(PATH, attempt.logPath)} - " +
- log"trying again...")
+ log"trying again...", ioe)
Review Comment:
I agree with @robreeves about the mental model for logging level, but also
agree with @mridulm about the inconsistency of logging level. From this method
by itself, my 2 cents goes to WARN. But in the whole context, this method is
called only from loadDiskStore and it logs the non-retriable (or already
retried) exception as INFO, because it's not end of the world and there will be
another fallback to RocksDB.
If we prefer consistency then maybe INFO is better, but then this logging
level is strongly coupled with the caller's context, so it's like a trade-off /
preference. Flipping the coin, maybe fallback attempts may worth to log warn -
would it be loadDiskStore we need to change to WARN? I don't have a strong
preference.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
