Github user squito commented on a diff in the pull request: https://github.com/apache/spark/pull/21165#discussion_r184404291 --- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala --- @@ -287,6 +287,33 @@ private[spark] class Executor( notifyAll() } + /** + * Set executor runtime and JVM gc time if task instance is still valid + */ + private def reportGCAndExecutorTimeIfPossible(taskStart: Long): Unit = { + if (task != null) { + task.metrics.setExecutorRunTime(System.currentTimeMillis() - taskStart) + task.metrics.setJvmGCTime(computeTotalGcTime() - startGCTime) + } + } + + /** + * Utility function to: + * 1. Report executor runtime and JVM gc time if possible + * 2. Collect accumulator updates + * 3. Set the finished flag to true and clear current thread's interrupt status + */ + private def collectAccumulatorsAndResetStatusOnFailure(taskStart: Long) = { + reportGCAndExecutorTimeIfPossible(taskStart) --- End diff -- I don't think the extra `reportGCAndExecutorTimeIfPossible` is necessary, you can just inline it. and also the original `if (task != null)` is probably easier to follow than `Option(task).map`
--- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org