SEZ9 commented on PR #11495: URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6228878317
Thanks for the detailed walkthrough of `9c047310570519504bd5d37be03421b46b377dba`. I have not gone through the diff myself yet, so I am treating the points below as to-be-confirmed rather than verified: **F1 / F2** – If the restore path now sends `CleanTaskGroupContextOperation` to the owned slots on live members before removing the pipeline snapshots from the master store, that ordering addresses the core of both findings. As noted above, a clean that fails on a worker that is still alive leaves that `TaskGroupContext` in place, so it can re-publish the previous attempt's counters after the wipe. Please add one sentence stating that case explicitly in the PR description (and ideally the method Javadoc), so the change is presented as a best-effort fix rather than a complete one. **F3** – If the behavior change is documented in the incompatible-changes page (en and zh) and the Zeta REST API job-lifecycle page as described, that covers my concern. I will confirm against the changed files. **F4** – An `InOrder` test for clean-before-remove and a test seeding the real `MetricsSnapshotStateStore` are what I was asking for. The "late report from the old attempt does not reappear" scenario would still be uncovered; given the best-effort design I will not block on it, but if it is cheap to add I would welcome it here, otherwise a short follow-up note is fine. **F5 / F6 / F7** – I do not see these covered in the thread. Could you confirm whether the `CALLS_REAL_METHODS` plus reflective field-injection scaffolding, the fully-qualified `java.lang.reflect.*` / `org.mockito.Mockito.*` references, and the duplication between `clearPipelineMetricsForRestore` and `removeMetricsContext` were touched in this head? Consolidating the two helpers and switching to imports should be quick; the test scaffolding can be a follow-up. On where the clear is called in `prepareRestorePipeline`: if `reset()` has no other callers, the two placements behave the same today, and I am fine keeping the call alongside the other restore-path steps. That is a style choice and yours to make. To summarize, before I approve: (1) the one-sentence failed-clean-on-live-worker note in the description/Javadoc, and (2) a quick status on F5–F7. <!-- streview-comment:1817 --> -- 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]
