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. Maybe fallback attempts may worth to log warn - it is 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: [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]
