yangzhang75 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4066049588
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -194,6 +202,30 @@ export class ExecuteWorkflowService {
return this.currentState;
}
+ /** How long the current run has been going, for a view that mounted after
it started. */
+ public getExecutionDuration(): number {
Review Comment:
You are right, and so is the reading of the engine -- I checked
`ExecutionStatsService.scala:153-175` and
`WorkflowExecutionService.scala:158-160` against your description and they say
what you say: the event fires only when a timestamp changes, and the start one
carries `currentTime - startTimeStamp` computed immediately after that
timestamp is written. So the cached number is ~0 for the whole run, and the
ticker sitting downstream of the event never starts for a view that mounted in
between. The clock was frozen at 0s, not merely seeded wrong.
And the test was circular, exactly as you describe: the harness stubbed the
getter, the test set it to 42000 and asserted 42000 came back. It covered the
form reading a getter.
Done the way you suggested, both halves. `ExecuteWorkflowService` keeps
`{reported, at, isRunning}` and answers `reported + (now - at)` while running,
and ticks it on a replaying stream that both views subscribe to -- so the
duplicated `timer(1000, 1000)` is gone from the menu and the Form View, and a
view mounting mid-run is handed the current value on subscribe and keeps
counting. The clock resets with the execution state too, which was @copilot's
separate point about the next workflow inheriting the last run's time.
The tests moved with the behaviour: the anchoring, the ticking, the end of a
run and the reset are covered on the service against real timers, and each view
keeps one test that it shows what the service says. Deletion-checked, including
going back to returning the snapshot, which is what I had. ba15011
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -95,6 +95,8 @@ export class ExecuteWorkflowService {
// TODO: move this to another service, or redesign how this
// information is stored on the frontend.
private assignedWorkerIds: Map<string, readonly string[]> = new Map();
+ /** The last duration the backend reported for the current run; see
getExecutionDuration. */
+ private executionDuration = 0;
Review Comment:
Fixed. The clock is reset with the execution state -- `resetExecutionState`
re-anchors it at zero, and `resetExecutionAndWorkers` goes through it -- so the
next workflow no longer opens showing the previous run's time. Covered on the
service spec. ba15011
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -194,6 +202,30 @@ export class ExecuteWorkflowService {
return this.currentState;
}
+ /** How long the current run has been going, for a view that mounted after
it started. */
+ public getExecutionDuration(): number {
+ return this.executionDuration;
Review Comment:
Right, and the snapshot was the wrong shape rather than a value to clear at
the right moment. @mengw15 traced it to the engine in the neighbouring thread:
the event carries a few milliseconds at the start of a run and nothing more
until it ends, so the cache held ~0 throughout and each view's ticker, sitting
downstream of that event, never started after a hand-over.
The service keeps an anchor now -- reported duration, when it was reported,
whether the run is going -- answers `reported + (now - at)` while running, and
ticks it on a replaying stream both views subscribe to. That is the "expose a
replaying/ticking duration" you asked for, and it also removes the duplicated
timers. ba15011
--
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]
