You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "PG1204 (via GitHub)" <gi...@apache.org> on 2026/08/31 22:23:46 UTC
[PR] refactor(frontend): separate operator state and statistics in WorkflowStatusService [texera]
PG1204 opened a new pull request, #8301:
URL: https://github.com/apache/texera/pull/8301
### What changes were proposed in this PR?
`WorkflowStatusService` currently bundles two different concepts in one object: `OperatorStatistics` carries both the operator's execution **state** (Running, Completed, …) and its **statistics** (row counts, sizes, timing). This PR splits them into separate sub-concepts, so the service now exposes three cleanly separated things: state, statistics, and performance metrics (the third was already separate, from #5834).
- `WorkflowStatusService` now has a stream + snapshot pair per concept: `getStateUpdateStream()` / `getCurrentState()` for state, and `getStatisticsUpdateStream()` / `getCurrentStatistics()` for statistics (metrics only). The performance-metrics API is unchanged.
- `OperatorStatistics` no longer contains `operatorState`. The combined shape the engine still sends over the websocket is typed as `OperatorRuntimeStatus`, and the service splits each update into the two maps. No backend or wire-format changes.
- All consumers are migrated: components that only cared about state (result panel, code debugger, UDF debug service, property editor) now read the state stream; the workflow editor renders state (operator color) and statistics (port counts, worker count) from their own streams. `JointUIService.changeOperatorStatistics` renders statistics only — state rendering stays in `changeOperatorState` (its two long-unused `isSource`/`isSink` params are dropped along the way).
- A small `WorkflowGraph.getAllOperatorIDs()` accessor keeps the per-update rendering path from materializing full operator predicates when only IDs are needed.
The change is behavior-preserving. One deliberate exception: the old code applied the "Recovering" display state by mutating the shared emitted map, which leaked masked states to other subscribers depending on subscription order. That accident is removed, and the override is now applied explicitly where state is rendered.
Rebased on top of the merged heat-map overlay (#6213), which consumes only the unchanged performance-metrics stream; its editor wiring is untouched by this refactor.
### Any related issues, documentation, discussions?
Closes #5919. Part of umbrella #5772. Follow-up from the review discussion in #5834; follows RFC discussion #5216.
### How was this PR tested?
The `WorkflowStatusService` spec now asserts state and statistics are exposed and update independently, and that statistics never leak `operatorState`. Consumer specs were updated to the new API, plus new tests for the state-rendering rules in the workflow editor (Uninitialized fallback, Recovering override, state label restored after navigation).
Full frontend suite passes (5,350 tests, 209 files); `tsc --noEmit`, `eslint ./src`, and Prettier are all clean.
### Was this PR authored or co-authored using generative AI tooling?
This PR was co-authored using Claude in compliance with ASF.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3925803848
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -1199,14 +1206,14 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy
/**
* Applies the validation result to the operator's border. Delegates to
- * applyOperatorBorder so validation, cached-execution-status, and the
+ * applyOperatorStateAndBorder so validation, cached-execution-status, and the
Review Comment:
addressed in [9f2c280](https://github.com/apache/texera/pull/8301/commits/9f2c2807a3b9cf729bdf29dfcd1eac7c0cbb40bf)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang merged PR #8301:
URL: https://github.com/apache/texera/pull/8301
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3918322155
##########
frontend/src/app/workspace/service/workflow-status/workflow-status.service.ts:
##########
@@ -27,37 +27,71 @@ import { OperatorPerformanceMetrics, extractPerformanceMetrics } from "./perform
providedIn: "root",
})
export class WorkflowStatusService {
- // status is responsible for passing websocket responses to other components
- private statusSubject = new Subject<Record<string, OperatorStatistics>>();
- private currentStatus: Record<string, OperatorStatistics> = {};
+ // The engine streams operator state and operator statistics bundled in one
+ // wire object (OperatorRuntimeStatus); this service splits them into two
+ // separate sub-concepts, each with its own stream and snapshot. Derived
+ // performance metrics are the third, separate concept.
+ private stateSubject = new Subject<Record<string, OperatorState>>();
+ private currentState: Record<string, OperatorState> = {};
+
+ private statisticsSubject = new Subject<Record<string, OperatorStatistics>>();
+ private currentStatistics: Record<string, OperatorStatistics> = {};
// Derived, ground-truth performance metrics for the heat-map overlay. Backed by
// a BehaviorSubject so a consumer that subscribes after a run already streamed
// (e.g. the overlay toggled on mid/post-run) still receives the latest value.
private performanceMetricsSubject = new BehaviorSubject<Record<string, OperatorPerformanceMetrics>>({});
constructor(private workflowWebsocketService: WorkflowWebsocketService) {
- // Single derivation path: every status emission (websocket, reset, clear, or
- // externally fed historical stats) recomputes the performance metrics.
- this.getStatusUpdateStream().subscribe(status => {
- this.currentStatus = status;
- this.performanceMetricsSubject.next(this.buildPerformanceMetrics(status));
+ this.getStateUpdateStream().subscribe(state => {
+ this.currentState = state;
+ });
+
+ // Single derivation path: every statistics emission (websocket, reset, or
+ // clear) recomputes the performance metrics.
+ this.getStatisticsUpdateStream().subscribe(statistics => {
+ this.currentStatistics = statistics;
+ this.performanceMetricsSubject.next(this.buildPerformanceMetrics(statistics));
});
+ // Each wire event produces exactly one emission on each stream, state
+ // first and statistics second (resetStatus/clearStatus follow the same
+ // order), so a statistics subscriber may read getCurrentState() and see
Review Comment:
addressed in [52e1942](https://github.com/apache/texera/pull/8301/commits/52e19424c8ad57a46807ee1d3d123a886fd3177e)
PR title modified.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3922288849
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -1199,14 +1206,14 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy
/**
* Applies the validation result to the operator's border. Delegates to
- * applyOperatorBorder so validation, cached-execution-status, and the
+ * applyOperatorStateAndBorder so validation, cached-execution-status, and the
Review Comment:
"cached-execution-status" is the pre-split umbrella word. The helper reads `getCurrentState()`, and its own doc at `:573` already says "cached execution state". My miss from the earlier rounds, not something this commit introduced.
```suggestion
* applyOperatorStateAndBorder so validation, cached-execution-state, and the
```
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3927209232
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -898,21 +899,15 @@ describe("WorkflowEditorComponent", () => {
* default (gray) when the user navigates away from and back to a workflow
* that has already finished executing. Both the operator-add stream and
* the validation stream route their final border decision through
- * applyOperatorBorder, which encodes the priority: invalid > cached
+ * applyOperatorStateAndBorder, which encodes the priority: invalid > cached
* execution state > default valid. These tests assert the operator's
* actual final rect.body/stroke on the paper, so they pin down the visible
* outcome rather than the internal helper calls.
*/
describe("operator border restoration after navigation", () => {
let workflowStatusService: WorkflowStatusService;
const cachedStatus = (operatorState: OperatorState) => ({
Review Comment:
`cachedStatus` builds a `Record<string, OperatorState>` and is mocked onto `getCurrentState`, and the five titles below it (`:920`, `:930`, `:942`, `:952`, `:962`) say "cached ... status".
The PR deliberately keeps "status" for the bundled concept — `handleOperatorStatusUpdate`, `OperatorRuntimeStatus` — which is what makes it the wrong word for the state half here. `cachedState` and "cached ... state" match what the helper actually returns.
##########
frontend/src/app/workspace/types/execute-workflow.interface.ts:
##########
@@ -93,9 +92,21 @@ export interface OperatorStatistics
aggregatedIdleTime?: number;
}> {}
+/**
+ * Wire shape of one operator's entry in OperatorStatisticsUpdateEvent. The
Review Comment:
This doc makes `OperatorRuntimeStatus` the wire shape, but `performance-metrics.ts:26` still calls `OperatorStatistics` "the raw ... the backend streams over the websocket". After the split that type is the frontend-side projection, so the two comments now disagree about what comes off the socket.
I cleared this in round 1 because every field `OperatorStatistics` declares is still backend-streamed — that reasoning does not survive the doc you added here.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #8301:
URL: https://github.com/apache/texera/pull/8301#issuecomment-5691838564
>
> @Yicong-Huang thanks for all the reviews, is this PR good to merge or is there anything else that needs to be addressed?
I always hope a second reviewer can give some more perspectives. I'm adding this to merge queue.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on PR #8301:
URL: https://github.com/apache/texera/pull/8301#issuecomment-5520608392
> 🟡 **1 resolved · 1 open · 0 new**
>
> The rework lands the fix: the snapshot assertion now runs where a failure can fail the test. Nothing new this round, and the only open item is the naming note carried over on unchanged code.
>
> ### Design & architecture (1)
> * `workflow-editor.component.ts:586` — `applyOperatorBorder` still repaints the full execution-state rendering, not just the border (advisory, carried over from the last two rounds)
>
> Verification trace
> Checked the fix rather than the reply. The assertion is now outside the subscriber (`workflow-status.service.spec.ts:120`), with only the capture left inside at `:112`, so a failure surfaces synchronously instead of escaping as an unhandled rxjs error. Traced both ways the guarantee can break: if the two `next()` calls swap, or if the state snapshot is never written, `currentState` is still the constructor's initial `{}` at capture time, so the captured value is `{}` and the assertion fails. The capture-by-reference is sound because the service reassigns `this.currentState = state` rather than mutating it, so the captured object is the one emitted for this event.
>
> Nothing else moved: the delta is one file, +8/−3, and the other 17 changed files are byte-identical to the last reviewed head. Re-diffed the two smallest consumers against the merge base this round — `udf-debug.service.ts:159` and `result-table-frame.component.ts:157` both replace `map[id]?.operatorState === X` with `map[id] === X`, equivalent including for an absent operator — and confirmed the operator-add path's `if (statistics)` guard already existed in the base, so the new early return in `changeOperatorStatistics` replaces a branch that was unreachable there. No reference to `getStatusUpdateStream` / `getCurrentStatus` / `currentOperatorStatus` survives in `frontend/src` or `agent-service/src`.
The full-state repaint is deliberate and is what fixes the state label vanishing after navigating away and back (an invalid operator previously kept only its red stroke on restore, pinned by the "restores the execution-state label" spec). But you're right that the name stopped matching the job two rounds ago, which has been renamed to applyOperatorStateAndBorder.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): separate operator state and statistics in WorkflowStatusService [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on PR #8301:
URL: https://github.com/apache/texera/pull/8301#issuecomment-5485881616
/request-review @Yicong-Huang
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): separate operator state and statistics in WorkflowStatusService [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8301:
URL: https://github.com/apache/texera/pull/8301#issuecomment-5485566382
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@aglinxinyuan`, `@mengw15`, `@Yicong-Huang`
You can notify them by mentioning `@aglinxinyuan`, `@mengw15`, `@Yicong-Huang` in a comment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3919117431
##########
frontend/src/app/workspace/service/workflow-status/workflow-status.service.spec.ts:
##########
@@ -58,32 +63,77 @@ describe("WorkflowStatusService", () => {
service = TestBed.inject(WorkflowStatusService);
});
- it("forwards an OperatorStatisticsUpdateEvent to the status stream", () => {
- const received: Record<string, OperatorStatistics>[] = [];
- service.getStatusUpdateStream().subscribe(s => received.push(s));
+ it("splits an OperatorStatisticsUpdateEvent into the state and statistics streams", () => {
+ const stateEmissions: Record<string, OperatorState>[] = [];
+ const statisticsEmissions: Record<string, OperatorStatistics>[] = [];
+ service.getStateUpdateStream().subscribe(s => stateEmissions.push(s));
+ service.getStatisticsUpdateStream().subscribe(s => statisticsEmissions.push(s));
- websocketEventSubject.next(statsEvent({ op1: sampleStats }));
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
- expect(received).toHaveLength(1);
- expect(received[0]).toEqual({ op1: sampleStats });
- expect(service.getCurrentStatus()).toEqual({ op1: sampleStats });
+ expect(stateEmissions).toHaveLength(1);
+ expect(stateEmissions[0]).toEqual({ op1: OperatorState.Running });
+ expect(statisticsEmissions).toHaveLength(1);
+ expect(statisticsEmissions[0]).toEqual({ op1: sampleStatistics });
+ expect(service.getCurrentState()).toEqual({ op1: OperatorState.Running });
+ expect(service.getCurrentStatistics()).toEqual({ op1: sampleStatistics });
+ });
+
+ it("does not leak the operator state into the statistics concept", () => {
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
+ expect(service.getCurrentStatistics()["op1"]).not.toHaveProperty("operatorState");
+ });
+
+ it("exposes state and statistics as independent streams", () => {
+ // A consumer subscribed to only one of the two sub-concepts sees exactly
+ // one emission per update, unaffected by the other stream.
+ let stateCount = 0;
+ let statisticsCount = 0;
+ service.getStateUpdateStream().subscribe(() => stateCount++);
+ service.getStatisticsUpdateStream().subscribe(() => statisticsCount++);
+
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
+ websocketEventSubject.next(statsEvent({ op1: { ...sampleRuntimeStatus, operatorState: OperatorState.Paused } }));
+
+ expect(stateCount).toBe(2);
+ expect(statisticsCount).toBe(2);
+ expect(service.getCurrentState()).toEqual({ op1: OperatorState.Paused });
+ });
+
+ it("emits state before statistics, so a statistics subscriber sees the matching state snapshot", () => {
+ const order: string[] = [];
+ service.getStateUpdateStream().subscribe(() => order.push("state"));
+ service.getStatisticsUpdateStream().subscribe(() => {
+ order.push("statistics");
+ // The licensed read: by the time statistics arrive, the state snapshot
+ // already reflects the same wire event. The converse does not hold.
+ expect(service.getCurrentState()).toEqual({ op1: OperatorState.Running });
+ });
+
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
+
+ expect(order).toEqual(["state", "statistics"]);
Review Comment:
reworked to capture the snapshot in the subscriber and assert after next() returns, with a comment noting why the assertion can't live in the callback. Both the order and the snapshot guarantee now fail the test synchronously if broken.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on PR #8301:
URL: https://github.com/apache/texera/pull/8301#issuecomment-5691811700
> 🟢 **2 resolved · 0 open · 0 new**
>
> Both terminology items land, and the sweep is now complete: no `cachedStatus`, no `registerStatusChangeHandler`, and exactly one type described as the wire shape. Nothing outstanding on this PR.
>
> Verification trace
@Yicong-Huang thanks for all the reviews, is this PR good to merge or is there anything else that needs to be addressed?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): separate operator state and statistics in WorkflowStatusService [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3911019286
##########
frontend/src/app/workspace/service/workflow-status/workflow-status.service.ts:
##########
@@ -27,37 +27,71 @@ import { OperatorPerformanceMetrics, extractPerformanceMetrics } from "./perform
providedIn: "root",
})
export class WorkflowStatusService {
- // status is responsible for passing websocket responses to other components
- private statusSubject = new Subject<Record<string, OperatorStatistics>>();
- private currentStatus: Record<string, OperatorStatistics> = {};
+ // The engine streams operator state and operator statistics bundled in one
+ // wire object (OperatorRuntimeStatus); this service splits them into two
+ // separate sub-concepts, each with its own stream and snapshot. Derived
+ // performance metrics are the third, separate concept.
+ private stateSubject = new Subject<Record<string, OperatorState>>();
+ private currentState: Record<string, OperatorState> = {};
+
+ private statisticsSubject = new Subject<Record<string, OperatorStatistics>>();
+ private currentStatistics: Record<string, OperatorStatistics> = {};
// Derived, ground-truth performance metrics for the heat-map overlay. Backed by
// a BehaviorSubject so a consumer that subscribes after a run already streamed
// (e.g. the overlay toggled on mid/post-run) still receives the latest value.
private performanceMetricsSubject = new BehaviorSubject<Record<string, OperatorPerformanceMetrics>>({});
constructor(private workflowWebsocketService: WorkflowWebsocketService) {
- // Single derivation path: every status emission (websocket, reset, clear, or
- // externally fed historical stats) recomputes the performance metrics.
- this.getStatusUpdateStream().subscribe(status => {
- this.currentStatus = status;
- this.performanceMetricsSubject.next(this.buildPerformanceMetrics(status));
+ this.getStateUpdateStream().subscribe(state => {
+ this.currentState = state;
+ });
+
+ // Single derivation path: every statistics emission (websocket, reset, or
+ // clear) recomputes the performance metrics.
+ this.getStatisticsUpdateStream().subscribe(statistics => {
+ this.currentStatistics = statistics;
+ this.performanceMetricsSubject.next(this.buildPerformanceMetrics(statistics));
});
+ // Each wire event produces exactly one emission on each stream, state
+ // first and statistics second (resetStatus/clearStatus follow the same
+ // order), so a statistics subscriber may read getCurrentState() and see
Review Comment:
This comment licenses consumers to rely on the emission order — "a statistics subscriber may read `getCurrentState()` and see the matching snapshot" — and the claim is true today, but nothing pins it. Swapping the two `next()` calls would break every consumer that took the licence, with no test failing.
It is also one-directional: a state subscriber reading the statistics snapshot gets the previous one. Worth saying, since the comment reads as a symmetric guarantee.
A three-line spec asserting the order would make the contract enforceable rather than advisory.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "PG1204 (via GitHub)" <gi...@apache.org>.
PG1204 commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3930546770
##########
frontend/src/app/workspace/types/execute-workflow.interface.ts:
##########
@@ -93,9 +92,21 @@ export interface OperatorStatistics
aggregatedIdleTime?: number;
}> {}
+/**
+ * Wire shape of one operator's entry in OperatorStatisticsUpdateEvent. The
Review Comment:
addressed in [f55bf88](https://github.com/apache/texera/pull/8301/commits/f55bf88f1795b7f930cfbd7b2ac4a2f045e88156)
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -898,21 +899,15 @@ describe("WorkflowEditorComponent", () => {
* default (gray) when the user navigates away from and back to a workflow
* that has already finished executing. Both the operator-add stream and
* the validation stream route their final border decision through
- * applyOperatorBorder, which encodes the priority: invalid > cached
+ * applyOperatorStateAndBorder, which encodes the priority: invalid > cached
* execution state > default valid. These tests assert the operator's
* actual final rect.body/stroke on the paper, so they pin down the visible
* outcome rather than the internal helper calls.
*/
describe("operator border restoration after navigation", () => {
let workflowStatusService: WorkflowStatusService;
const cachedStatus = (operatorState: OperatorState) => ({
Review Comment:
addressed in [f55bf88](https://github.com/apache/texera/pull/8301/commits/f55bf88f1795b7f930cfbd7b2ac4a2f045e88156)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): split operator state from statistics [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8301:
URL: https://github.com/apache/texera/pull/8301#discussion_r3919022178
##########
frontend/src/app/workspace/service/workflow-status/workflow-status.service.spec.ts:
##########
@@ -58,32 +63,77 @@ describe("WorkflowStatusService", () => {
service = TestBed.inject(WorkflowStatusService);
});
- it("forwards an OperatorStatisticsUpdateEvent to the status stream", () => {
- const received: Record<string, OperatorStatistics>[] = [];
- service.getStatusUpdateStream().subscribe(s => received.push(s));
+ it("splits an OperatorStatisticsUpdateEvent into the state and statistics streams", () => {
+ const stateEmissions: Record<string, OperatorState>[] = [];
+ const statisticsEmissions: Record<string, OperatorStatistics>[] = [];
+ service.getStateUpdateStream().subscribe(s => stateEmissions.push(s));
+ service.getStatisticsUpdateStream().subscribe(s => statisticsEmissions.push(s));
- websocketEventSubject.next(statsEvent({ op1: sampleStats }));
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
- expect(received).toHaveLength(1);
- expect(received[0]).toEqual({ op1: sampleStats });
- expect(service.getCurrentStatus()).toEqual({ op1: sampleStats });
+ expect(stateEmissions).toHaveLength(1);
+ expect(stateEmissions[0]).toEqual({ op1: OperatorState.Running });
+ expect(statisticsEmissions).toHaveLength(1);
+ expect(statisticsEmissions[0]).toEqual({ op1: sampleStatistics });
+ expect(service.getCurrentState()).toEqual({ op1: OperatorState.Running });
+ expect(service.getCurrentStatistics()).toEqual({ op1: sampleStatistics });
+ });
+
+ it("does not leak the operator state into the statistics concept", () => {
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
+ expect(service.getCurrentStatistics()["op1"]).not.toHaveProperty("operatorState");
+ });
+
+ it("exposes state and statistics as independent streams", () => {
+ // A consumer subscribed to only one of the two sub-concepts sees exactly
+ // one emission per update, unaffected by the other stream.
+ let stateCount = 0;
+ let statisticsCount = 0;
+ service.getStateUpdateStream().subscribe(() => stateCount++);
+ service.getStatisticsUpdateStream().subscribe(() => statisticsCount++);
+
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
+ websocketEventSubject.next(statsEvent({ op1: { ...sampleRuntimeStatus, operatorState: OperatorState.Paused } }));
+
+ expect(stateCount).toBe(2);
+ expect(statisticsCount).toBe(2);
+ expect(service.getCurrentState()).toEqual({ op1: OperatorState.Paused });
+ });
+
+ it("emits state before statistics, so a statistics subscriber sees the matching state snapshot", () => {
+ const order: string[] = [];
+ service.getStateUpdateStream().subscribe(() => order.push("state"));
+ service.getStatisticsUpdateStream().subscribe(() => {
+ order.push("statistics");
+ // The licensed read: by the time statistics arrive, the state snapshot
+ // already reflects the same wire event. The converse does not hold.
+ expect(service.getCurrentState()).toEqual({ op1: OperatorState.Running });
+ });
+
+ websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
+
+ expect(order).toEqual(["state", "statistics"]);
Review Comment:
rxjs 7.8.2 does not let the `next()` caller catch a throw from a subscriber — it re-throws asynchronously — so this assertion cannot fail the test it sits in. If the snapshot guarantee broke, the order assertion below would still pass and the failure would surface detached, as an unhandled error. It is the same hazard the flag at `workflow-editor.component.spec.ts:1963` works around; capturing the snapshot and asserting after `next()` returns avoids needing the flag at all.
```suggestion
const order: string[] = [];
let stateAtStatistics: Record<string, OperatorState> | undefined;
service.getStateUpdateStream().subscribe(() => order.push("state"));
service.getStatisticsUpdateStream().subscribe(() => {
order.push("statistics");
// The licensed read: by the time statistics arrive, the state snapshot
// already reflects the same wire event. The converse does not hold.
stateAtStatistics = service.getCurrentState();
});
websocketEventSubject.next(statsEvent({ op1: sampleRuntimeStatus }));
expect(order).toEqual(["state", "statistics"]);
expect(stateAtStatistics).toEqual({ op1: OperatorState.Running });
```
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(frontend): separate operator state and statistics in WorkflowStatusService [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8301:
URL: https://github.com/apache/texera/pull/8301#issuecomment-5485650717
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/8301?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `98.24561%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 94.09%. Comparing base ([`7d57cd6`](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9a44593`](https://app.codecov.io/gh/apache/texera/commit/9a44593e3b6b7e3665d3561028ae0298dd010b23?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/texera/pull/8301?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...onent/workflow-editor/workflow-editor.component.ts](https://app.codecov.io/gh/apache/texera/pull/8301?src=pr&el=tree&filepath=frontend%2Fsrc%2Fapp%2Fworkspace%2Fcomponent%2Fworkflow-editor%2Fworkflow-editor.component.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZnJvbnRlbmQvc3JjL2FwcC93b3Jrc3BhY2UvY29tcG9uZW50L3dvcmtmbG93LWVkaXRvci93b3JrZmxvdy1lZGl0b3IuY29tcG9uZW50LnRz) | 94.11% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/texera/pull/8301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8301 +/- ##
============================================
- Coverage 94.09% 94.09% -0.01%
Complexity 4806 4806
============================================
Files 1193 1193
Lines 48493 48512 +19
Branches 5858 5860 +2
============================================
+ Hits 45631 45646 +15
- Misses 1420 1421 +1
- Partials 1442 1445 +3
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.00% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `99.32% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.10% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.67% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.00% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.91% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `96.86% <98.24%> (-0.02%)` | :arrow_down: | |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.31% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.18% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/8301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.19% <ø> (ø)` | | Carriedforward from [7d57cd6](https://app.codecov.io/gh/apache/texera/commit/7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/8301?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org