Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20940#discussion_r180114405
  
    --- Diff: 
core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
    @@ -234,8 +244,22 @@ private[spark] class EventLoggingListener(
         }
       }
     
    -  // No-op because logging every update would be overkill
    -  override def onExecutorMetricsUpdate(event: 
SparkListenerExecutorMetricsUpdate): Unit = { }
    +  /**
    +   * Log if there is a new peak value for one of the memory metrics for 
the given executor.
    +   * Metrics are cleared out when a new stage is started in 
onStageSubmitted, so this will
    +   * log new peak memory metric values per executor per stage.
    +   */
    +  override def onExecutorMetricsUpdate(event: 
SparkListenerExecutorMetricsUpdate): Unit = {
    --- End diff --
    
    You could tell which stages are active from the other events.  Do you think 
that timeline would be useful?  Since its only keeping the peak, I'm not sure 
how interesting that graph is.
    
    With overlapping stages, you might lose some information by only logging at 
stage end -- eg. if first stage 1 is running for a while, with peak X, and then 
stage 2 starts up with peak Y > X, you would only ever log peak Y for stage 1.  
You could address this by also logging at stage start, but then you're back to 
more logging (and its really tricky to figure out a minimal set of things of 
log when stage 2 starts, as you don't know which executor its going to run on).
    
    But I'm still not sure what we'd do with those extra values, and if there 
is any value in capturing them.
    
    (some this design discussion probably belongs on jira -- I'll also go 
reread the design doc now)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to