dongjoon-hyun commented on code in PR #58968:
URL: https://github.com/apache/spark/pull/58968#discussion_r4080318401
##########
core/src/main/scala/org/apache/spark/status/LiveEntity.scala:
##########
@@ -379,34 +379,60 @@ private class LiveExecutorStageSummary(
attemptId: Int,
executorId: String) extends LiveEntity {
- import LiveEntityHelpers._
-
var taskTime = 0L
var succeededTasks = 0
var failedTasks = 0
var killedTasks = 0
var isExcluded = false
- var metrics = createMetrics(default = 0L)
+ // Only the longs that ExecutorStageSummary exposes. Do not hold a
v1.TaskMetrics graph
+ // (Input/Output/ShuffleRead/ShuffleWrite/ShufflePushRead) per (stage,
executor).
+ var inputBytes = 0L
+ var inputRecords = 0L
+ var outputBytes = 0L
+ var outputRecords = 0L
+ var shuffleRead = 0L
+ var shuffleReadRecords = 0L
+ var shuffleWrite = 0L
+ var shuffleWriteRecords = 0L
+ var memoryBytesSpilled = 0L
+ var diskBytesSpilled = 0L
val peakExecutorMetrics = new ExecutorMetrics()
+ def addTaskMetrics(delta: v1.TaskMetrics): Unit = {
+ if (delta == null) {
Review Comment:
This null check seems unnecessary. `onTaskEnd` already guards with `if
(metricsDelta != null)`, and in `onExecutorMetricsUpdate` the delta comes from
`TaskMetrics.fromAccumulatorInfos`, which is never null. The previous code
didn't handle null here either. Could we remove it? It also lets us avoid
`return`, which the Databricks Scala style guide discourages.
##########
core/src/test/scala/org/apache/spark/status/AppStatusListenerSuite.scala:
##########
@@ -1917,6 +1917,12 @@ abstract class AppStatusListenerSuite extends
SparkFunSuite with BeforeAndAfter
checkInfoPopulated(listener, logUrlMap, processId)
}
+ test("LiveExecutorStageSummary does not hold v1.TaskMetrics") {
Review Comment:
This reflection-based test only checks the class structure, not the
behavior. Currently, `AppStatusListenerSuite` verifies only
`memoryBytesSpilled` of `ExecutorStageSummary`, so a wrong field mapping in
`addTaskMetrics` (e.g., input/output/shuffle read/shuffle write) would not be
caught. Could you add (or replace this with) a test which sends
`SparkListenerTaskEnd` and `SparkListenerExecutorMetricsUpdate` with distinct
metric values and verifies all 10 metric fields of `ExecutorStageSummary`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
