mridulm commented on code in PR #47794:
URL: https://github.com/apache/spark/pull/47794#discussion_r1720740196


##########
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 dont have a strong opinion in this specific case.
   
   This is a retriable failure, and could simply succeed on next attempt. Note 
that the exception, when if it gets bubbled up (`retried == true`), gets logged 
at `INFO` currently (see `loadDiskStore`) - so the `logWarning` level here, to 
begin with, looks slightly off.
   
   +CC @HeartSaVioR who reviewed the original PR



-- 
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

Reply via email to