Yicong-Huang commented on code in PR #6729:
URL: https://github.com/apache/texera/pull/6729#discussion_r3764523354
##########
amber/src/main/scala/org/apache/texera/web/service/ExecutionStatsService.scala:
##########
@@ -124,7 +124,8 @@ class ExecutionStatsService(
metrics.operatorStatistics.numWorkers,
metrics.operatorStatistics.dataProcessingTime,
metrics.operatorStatistics.controlProcessingTime,
- metrics.operatorStatistics.idleTime
+ metrics.operatorStatistics.idleTime,
+ reusedFromCache = metrics.reusedFromCache
Review Comment:
`computeStatsDiff` below (`:243-256`) rebuilds `OperatorMetrics` from
`operatorState` plus the six `OperatorStatistics` fields — an identity
transform before this PR, hence its "consistent structure" comment. With field
3 it silently resets `reusedFromCache` to `false`.
Nothing observes that today, since `runtimeStatisticsSchema` has no such
column. But whether that row carries the flag is a #5884 decision you name in
the description, and this block would feed it `false`. The rebuild is a no-op,
so deleting it is the cleanest fix.
##########
amber/src/test/scala/org/apache/texera/amber/engine/architecture/coordinator/execution/ExecutionUtilsSpec.scala:
##########
@@ -337,4 +339,27 @@ class ExecutionUtilsSpec extends AnyFlatSpec {
assert(result.operatorStatistics.numWorkers == 3)
assert(result.operatorStatistics.dataProcessingTime == 12)
}
+
+ // -- aggregateMetrics: reused-from-cache provenance ----------------------
+
+ it should "report reusedFromCache only when every physical operator is
reused" in {
+ val reusedA = metricsWith(WorkflowAggregatedState.COMPLETED, reused = true)
+ val reusedB = metricsWith(WorkflowAggregatedState.COMPLETED, reused = true)
+ val computed = metricsWith(WorkflowAggregatedState.COMPLETED)
+
+ assert(ExecutionUtils.aggregateMetrics(List(reusedA,
reusedB)).reusedFromCache)
+ assert(!ExecutionUtils.aggregateMetrics(List(reusedA,
computed)).reusedFromCache)
+ assert(!ExecutionUtils.aggregateMetrics(List(computed)).reusedFromCache)
+ }
+
+ it should "default reusedFromCache to false for empty input and untouched
metrics" in {
+ // Empty input takes the early-return path, whose default is false. This is
+ // the empty-cache property for the flag: nothing sets it until a producer
does.
Review Comment:
"the empty-cache property for the flag" is PR-history vocabulary — it reads
only to someone holding this PR's discussion, and stops meaning anything once
this merges.
```suggestion
// Empty input takes the early-return path, whose default is false;
metrics
// that no producer has marked keep that default too.
```
While you are here: the second assertion (`:359-363`) repeats `:352`. Both
are `forall` over a single `reused = false` element, and the differing state
does not affect the flag.
--
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]
