aglinxinyuan opened a new pull request, #7544: URL: https://github.com/apache/texera/pull/7544
### What changes were proposed in this PR? `ExecutionStatsService` had no spec and sat at **0% of its 80 lines**, while being the class that tells the frontend what an execution is doing: per-operator input/output metrics, worker assignment, elapsed duration, and the runtime statistics persisted for the dashboard's time series. Adds 12 tests driven through the three-argument constructor. The client is an `AmberClient` subclass that captures the registered callbacks, so the tests fire `ExecutionStatsUpdate`, `RuntimeStatisticsPersist`, `WorkerAssignmentUpdate`, `WorkflowRecoveryStatus`, `FatalError` and `ExecutionStateUpdate` directly; the state store is real, so events travel through the production diff handlers. This follows `ExecutionRuntimeServiceSpec` and `ExecutionConsoleServiceSpec`. The most valuable one is the carry-forward: an operator that stops reporting must still appear in the persisted statistics, or its row silently vanishes from the time series mid-execution. ### Verification 19 mutations applied and reverted, production diff empty after each. All red, including the positional column layout of the persisted tuple, the commit guard holding statistics back until a terminal state, `client.shutdown()` on `FatalError`, and the wid/eid argument order into `updateRuntimeStatsUri`. Three assertions were **found to be vacuous in review and strengthened**, which is the part worth reading: | Weakness | Why it passed | Fix | |---|---|---| | running-duration arithmetic unpinned | the test asserted only `duration >= 1500`, so `currentTime - start` becoming `currentTime + start` (~111 years) still passed | bounded on both sides with a window captured around the update | | "every operator" observed one operator | the fixture reported a single operator, so `operatorInfo.collect` could be narrowed to `.take(1).collect` | a second operator with different numbers | | "publish nothing" checked the payload, not the event | the helper flattened the event's map, so a present-but-empty churn event to the websocket was invisible | collect by event type and assert empty, matching the two sibling tests | All three mutations are now red. ### Deliberately not included - The `catch` around `runtimeStatsWriter.close()` — Iceberg's close is idempotent, so nothing reaches it without injecting a throwing writer. - The `catch` in `storeRuntimeStatistics` — it runs on a private single-thread executor, which swallows the throwable, so no assertion could observe the mutation. - Three dead lines in `computeStatsDiff` (`defaultMetrics`, `newKeys`, and the `++ newKeys.map(...)` merge). `updatedLastMetrics` is read only at `oldKeys.map(key => key -> updatedLastMetrics(key))`, and `oldKeys`/`newKeys` are disjoint by construction, so the merged entries can never be selected — confirmed by replacing the whole expression with `lastPersistedMetrics` and seeing all 12 tests stay green. Reported rather than cemented; deleting beats testing. Two notes for reviewers. The spec uses distinct workflow/execution ids because the runtime-statistics URI derives from them and `createDocument` overrides an existing table — a default `WorkflowContext` collides with `DefaultCostEstimatorSpec`, and sbt runs amber suites in parallel in one JVM. And no temp Iceberg catalog is installed: `IcebergCatalogInstance.replaceInstance` is JVM-wide and the URI carries no warehouse, so installing one would hijack the catalog for every other amber suite. No production file is touched. ### Any related issues, documentation, discussions? Closes #7542 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.ExecutionStatsServiceSpec" ``` ``` [info] Total number of tests run: 12 [info] Tests: succeeded 12, failed 0, canceled 0, ignored 0, pending 0 ``` The env var matches what CI's unit job already sets (`.github/workflows/build.yml:293`); the committed default expects a Lakekeeper. `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) -- 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]
