SEZ9 commented on PR #11495: URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6183582630
Looked at the follow-up on `9c047310` against the earlier `f45b6de63a` state, focusing only on the points from the previous round. **F1 / F2 (restore path and race with the worker report)** – The new ordering in `clearPipelineMetricsForRestore` looks like the right shape: clean the failed attempt's task-group contexts on the surviving workers via `CleanTaskGroupContextOperation` first, then drop the pipeline snapshots from the master store, all before `reset()` under `restoreLock`. Making `cleanTaskGroupContext` best-effort per task group (log and continue, re-set the interrupt flag on `InterruptedException`) instead of aborting the whole loop on the first failure is also an improvement, since one unreachable worker no longer leaves the other workers' stale contexts in place. One thing I'd like confirmed in the PR description: in the case where the clean operation fails for a worker that is still a member, what happens to that worker's `finishedExecutionContexts` — is it still possible for it to re-report the old attempt's counters after the snapshot has been removed? If so, please state that limitation e xplicitly so we don't claim a complete fix for #11472. **F3 (semantics change for streaming jobs)** – I see docs were touched in this revision. Please point me to the exact place where the "counters reset on pipeline retry" behaviour is described, and whether it is listed as an incompatible/behaviour change rather than only an inline note. **F4 / F5 (tests)** – Please confirm whether `StateTransitionCleanupTest` still only verifies that the mock is invoked, or whether a test now exercises the actual restore path (real metrics store, worker re-report, new attempt starting from zero). If it is still mock-only, an e2e or integration-style test covering the failover scenario from #11472 would be the strongest evidence that the race is actually closed. The `CALLS_REAL_METHODS` + reflective private-field injection setup is still something I'd prefer to see simplified if the test is kept. **F6 / F7 (style)** – Minor, but please confirm the fully-qualified `java.lang.reflect.*` / `org.mockito.Mockito.*` references were replaced with imports, and whether `clearPipelineMetricsForRestore` and `removeMetricsContext` have been consolidated into a single helper (or explain why they need to stay separate). Once the `Build` run finishes and the above are confirmed, I'm happy to do a final pass. <!-- streview-comment:1769 --> -- 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]
