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
