sarutak commented on code in PR #57751:
URL: https://github.com/apache/spark/pull/57751#discussion_r3712759502
##########
sql/core/src/main/scala/org/apache/spark/status/api/v1/sql/SqlResource.scala:
##########
@@ -200,12 +206,33 @@ private[v1] class SqlResource extends BaseAppResource {
case "duration" =>
execs.sortBy(e =>
e.completionTime.getOrElse(new Date()).getTime - e.submissionTime)
+ case "totalTaskTime" => execs.sortBy(e => totalTaskTime(e, store))
Review Comment:
When sorting by `totalTaskTime`, the value is computed for all executions
here, and then computed again for each execution on the current page in
`execToRow`. Each computation does KVStore lookups for all stage attempts.
With the default page size of 20, the duplicated computation is limited to
20 executions × their stages, so this is unlikely to be a practical issue.
`sortBy`'s Schwartzian transform ensures the sort itself computes the key only
once per element.
One approach would be to precompute a `Map[Long, Long]` (executionId ->
totalTaskTime) and reuse it
in both sort and row construction.
##########
sql/core/src/main/scala/org/apache/spark/status/api/v1/sql/SqlResource.scala:
##########
@@ -200,12 +206,33 @@ private[v1] class SqlResource extends BaseAppResource {
case "duration" =>
execs.sortBy(e =>
e.completionTime.getOrElse(new Date()).getTime - e.submissionTime)
+ case "totalTaskTime" => execs.sortBy(e => totalTaskTime(e, store))
case _ => execs.sortBy(_.executionId)
}
if (sortDir == "asc") sorted else sorted.reverse
}
- private def execToRow(exec: SQLExecutionUIData):
java.util.LinkedHashMap[String, Object] = {
+ /**
+ * Total task time of an execution, in milliseconds, aggregated across all
+ * stages of the execution. Sums `executorRunTime` (the "Total Time Across
All
+ * Tasks" metric) of every attempt of every stage: each attempt genuinely
+ * consumed task time, including failed attempts that were retried. Returns
-1
+ * when the execution has no stages to aggregate, so callers can distinguish
+ * "no task time information" from a genuine zero.
+ */
Review Comment:
The Scaladoc says:
```
Sums `executorRunTime` (the "Total Time Across All Tasks" metric)
```
`executorRunTime` specifically measures the time executors spent running
task code. It excludes deserialization time, result serialization time, and GC
time. The "Total Time Across All Tasks" label used on the Stages page is the
same metric, so the description isn't wrong, but a slightly more precise
phrasing might avoid confusion:
```
Sums `executorRunTime` (the cumulative time executors spent running tasks,
which is the "Total Time Across All Tasks" stage-level metric)
```
--
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]
