SEZ9 commented on PR #11495: URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6207573843
Thanks for the update on `9c047310570519504bd5d37be03421b46b377dba`. CI is no longer a concern from my side; what remains before I can approve is closing out the earlier review points, which I have not yet re-verified against this head. Could you confirm the current state of each of these, either in the PR description or in a reply here? 1. **Worker-side cleanup and the re-report race (F1, F2).** Does the restore path now send `CleanTaskGroupContextOperation` to surviving workers? If so, what happens when that operation fails on a still-alive worker, and what prevents a metrics report already in flight from re-populating the map before the new attempt deploys? If this case is not fully covered, please state the limitation plainly in the PR description so the change does not over-claim a complete fix. 2. **Counter semantics for streaming jobs (F3).** If counters now reset to zero on every pipeline retry by design, please add a short note in the docs / incompatible-changes section and reference it from the PR description. If that is not intended, the reset should be scoped. 3. **Test coverage (F4).** Do any of the new tests go beyond verifying a mock invocation? Ideally at least one test would use the real metrics store, exercise the worker re-report path, and assert that the restored attempt starts from zero and that a late report from the old attempt does not reappear. Non-blocking, but worth addressing in the same push if still applicable: F5 (avoid the partially-initialised `JobMaster` via `CALLS_REAL_METHODS` plus reflective private-field injection), F6 (use imports instead of fully-qualified `java.lang.reflect.*` / `org.mockito.Mockito.*` references), and F7 (consolidate `clearPipelineMetricsForRestore` and `removeMetricsContext` into one helper with a log-level or reason parameter). Once a new commit or description update lands covering items 1–3, I'll take another pass right away. <!-- streview-comment:1799 --> -- 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]
