Yicong-Huang commented on code in PR #6729:
URL: https://github.com/apache/texera/pull/6729#discussion_r3742252015
##########
frontend/src/app/workspace/types/execute-workflow.interface.ts:
##########
@@ -159,6 +159,7 @@ export enum ExecutionState {
Terminated = "Terminated",
Failed = "Failed",
Killed = "Killed",
+ CacheReused = "CacheReused",
Review Comment:
Correcting my earlier comment: adding `CacheReused` to the
`ExecutionStateInfo` terminal variant at :180 does not compile on its own.
`updateWorkflowActionLock` (execute-workflow.service.ts:385-404) ends in
`return exhaustiveGuard(stateInfo)`, and `exhaustiveGuard` takes `never`
(common/util/switch.ts:20). A new `ExecutionStateInfo` member leaves the
default branch narrowed to that member instead of `never`, so the call fails
TS2345 — one build error replacing the other.
The guard is doing exactly its job here: it is asking where a cache-reused
workflow belongs. Add `case ExecutionState.CacheReused:` beside
`Completed`/`Terminated` at :387 in the same change.
##########
amber/src/main/scala/org/apache/texera/amber/engine/common/Utils.scala:
##########
@@ -166,6 +168,7 @@ object Utils extends LazyLogging {
case WorkflowAggregatedState.COMPLETED => 3
case WorkflowAggregatedState.FAILED => 4
case WorkflowAggregatedState.KILLED => 5
+ case WorkflowAggregatedState.CACHE_REUSED => 6
Review Comment:
Correcting my earlier comment: keep this case even if the three dashboard
mirrors go.
`maptoStatusCode` has a second caller I missed.
`ExecutionStatsService.scala:294` writes `maptoStatusCode(stat.operatorState)`
into the runtime-statistics `status` column (ResultSchema.scala:36). That
surface is operator-level, so it does receive `CACHE_REUSED` once the state has
a producer; remove the case and it silently persists `-1`.
The three TS mirrors read `workflow_executions.status` and stay dead. #5883
also puts UI out of scope (#5886) — a second reason to drop just those three.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/coordinator/execution/ExecutionUtils.scala:
##########
@@ -50,7 +50,8 @@ object ExecutionUtils {
WorkflowAggregatedState.RUNNING,
WorkflowAggregatedState.UNINITIALIZED,
WorkflowAggregatedState.PAUSED,
- WorkflowAggregatedState.READY
+ WorkflowAggregatedState.READY,
+ Some(WorkflowAggregatedState.CACHE_REUSED)
Review Comment:
Nothing can put `CACHE_REUSED` into this input, and #5884 won't be able to
either without changing the layer below.
`aggregateMetrics` reads states produced by `OperatorExecution.getState`
(OperatorExecution.scala:73-84). That call passes no `cachedState`, and
`WorkerState` (statistics.proto:32-40) has no cached member to pass. A cached
region has zero workers — #5883 says so directly — so `workerStates` is empty
and `aggregateStates` returns `UNINITIALIZED` from its `states.isEmpty` branch.
The sentinel half fails the same way: `getStats` on a zero-worker operator
emits no port mappings, so nothing can carry a `-1`.
Both branches sit one layer above where the cached shape arrives. If #5884
has to synthesize `OperatorMetrics` for a skipped region anyway, that is where
this logic belongs.
##########
amber/src/test/scala/org/apache/texera/amber/engine/architecture/coordinator/execution/ExecutionUtilsSpec.scala:
##########
@@ -181,6 +274,61 @@ class ExecutionUtilsSpec extends AnyFlatSpec {
assert(ExecutionUtils.aggregatePortMetrics(List(mapping)) == Seq(mapping))
}
+ it should "mark a port unknown (-1 count and size) when a mapping has a
negative size" in {
+ // A negative in either field marks the whole port unknown, discarding a
+ // valid count. Size-only exercises the right operand of the check.
Review Comment:
"Size-only" is left without a noun to modify.
```suggestion
// valid count. A size-only negative exercises the right operand of the
check.
```
--
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]
