spark-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From squito <...@git.apache.org>
Subject [GitHub] spark pull request #21221: [SPARK-23429][CORE] Add executor memory metrics t...
Date Mon, 18 Jun 2018 15:00:41 GMT
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/21221#discussion_r196111195
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
    @@ -169,6 +182,31 @@ private[spark] class EventLoggingListener(
     
       // Events that trigger a flush
       override def onStageCompleted(event: SparkListenerStageCompleted): Unit = {
    +    if (shouldLogExecutorMetricsUpdates) {
    +      // clear out any previous attempts, that did not have a stage completed event
    +      val prevAttemptId = event.stageInfo.attemptNumber() - 1
    +      for (attemptId <- 0 to prevAttemptId) {
    +        liveStageExecutorMetrics.remove((event.stageInfo.stageId, attemptId))
    +      }
    +
    +      // log the peak executor metrics for the stage, for each live executor,
    +      // whether or not the executor is running tasks for the stage
    +      val accumUpdates = new ArrayBuffer[(Long, Int, Int, Seq[AccumulableInfo])]()
    +      val executorMap = liveStageExecutorMetrics.remove(
    +        (event.stageInfo.stageId, event.stageInfo.attemptNumber()))
    +      executorMap.foreach {
    +       executorEntry => {
    +          for ((executorId, peakExecutorMetrics) <- executorEntry) {
    +            val executorMetrics = new ExecutorMetrics(-1, peakExecutorMetrics.metrics)
    --- End diff --
    
    there is no point in logging the timestamp, if we only log -1.  Better to just remove
it.  Are you doing anything with it in the live UI?  Or should we just get rid of the timestamp
field entirely?  (it can always be added later if there is a use for it.)
    
    I agree that having one timestamp for the peak across all metrics isn't very useful. 
Its possible *all* the metrics would hit a peak at nearly the same time, but the more metrics
we add, the less likely that becomes.  And even if we had the timestamp for each metric, what
would we do with it?


---

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


Mime
View raw message