Yicong-Huang commented on code in PR #6729:
URL: https://github.com/apache/texera/pull/6729#discussion_r3771672957
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/coordinator/execution/ExecutionUtils.scala:
##########
@@ -77,7 +77,11 @@ object ExecutionUtils {
dataProcessingTimeSum,
controlProcessingTimeSum,
idleTimeSum
- )
+ ),
+ // A logical operator is reused from cache only when every one of its
+ // physical operators is. `metrics` is non-empty here, so this cannot
+ // hold vacuously.
+ reusedFromCache = metrics.forall(_.reusedFromCache)
Review Comment:
The comment promises more than the input set delivers. The only caller
(`WorkflowExecution.scala:94-110`) flattens `getAllRegionExecutions` and groups
by `logicalOpId`, so `metrics` holds the physical operators that currently
*have* a region execution. For a HashJoin, the probe operator contributes
nothing until its region is created, so `forall` can return true with a later
operator still to come. `SyncExecutionResource.scala:189-200` documents that
same window for the sibling state field, so this is the function's existing
shape rather than something you introduced.
Can a logical operator ever be *partially* reused under #5884? If reuse is
all-or-nothing, only the comment needs narrowing to "the physical operators
reporting so far". If it can be partial, the flag wants the same late-region
guard the state field got.
##########
amber/src/main/scala/org/apache/texera/web/model/websocket/event/OperatorStatisticsUpdateEvent.scala:
##########
@@ -30,7 +30,9 @@ case class OperatorAggregatedMetrics(
numWorkers: Long,
aggregatedDataProcessingTime: Long,
aggregatedControlProcessingTime: Long,
- aggregatedIdleTime: Long
+ aggregatedIdleTime: Long,
+ // Provenance: the operator completed by reusing cached results (no
workers ran).
+ reusedFromCache: Boolean = false
Review Comment:
This is the only one of the twelve fields with a default, and neither
construction site takes it — `ExecutionStatsService.scala:128` and
`TexeraWebSocketEventSpec.scala:129` both pass it explicitly. Its only live
effect is on the *next* construction site, where it turns a forgotten flag into
a silent `false` rather than a compile error. That is the shape of the
`computeStatsDiff` rebuild we just deleted, so it seems worth closing off while
it is free.
```suggestion
reusedFromCache: Boolean
```
--
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]
