SEZ9 commented on PR #11495: URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6137429813
Thanks @zhangshenghang. The ordering you describe for `f45b6de6` (`cleanTaskGroupContext(pipelineLocation)` before `seaTunnelServer.removeMetrics(...)` in `clearPipelineMetricsForRestore`) is the right shape for Issue 1, but I can't yet see the diff for that head, so I'm treating it as unverified until I can review the pushed change. The remaining items from my previous review are: 1. **Task-group context cleanup before the metrics clear (Issue 1).** To be confirmed against the `f45b6de6` diff, including that a cleanup failure is logged and never blocks the restore as you describe. 2. **Ordering vs. the periodic metrics report.** Even with the contexts cleaned first, a worker report snapshotted before the clean could still land after the clear. Could you explain how that window is handled (e.g. the clear runs only after the clean is acknowledged, or reports for an unregistered task group are ignored)? If it is still probabilistic, let's note that explicitly and decide whether it's acceptable. 3. **Semantics / docs note.** Counters that previously survived a pipeline retry via the IMap snapshot now reset to zero on every restore. Please add a short note to the Zeta metrics docs or the incompatible-changes list so streaming users aren't surprised. 4. **Test coverage.** Glad `StateTransitionCleanupTest` is green locally. The concern remains that the tests only verify a mocked method is invoked. It would be good to have at least one test that asserts `cleanTaskGroupContext` runs before `removeMetrics` (in-order verification) and one that exercises a real metrics store / re-report path and asserts the restored attempt actually starts from zero. If the latter is too heavy for a unit test, an engine IT is fine. 5. **Test scaffolding and style (minor).** The partially-initialised `JobMaster` via `CALLS_REAL_METHODS` plus reflective private-field injection is brittle; constructing the needed pieces directly or extracting the clear/clean logic into a small testable unit would help. Please also replace the fully-qualified `java.lang.reflect.*` / `org.mockito.Mockito.*` references with imports. 6. **Helper duplication (minor).** `clearPipelineMetricsForRestore` looks like a copy of `removeMetricsContext` with a different log level; please consolidate into one helper now that the restore path has an extra step. I'll re-review as soon as the `f45b6de6` changes are visible here. <!-- streview-comment:1715 --> -- 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]
