SEZ9 commented on PR #11495: URL: https://github.com/apache/seatunnel/pull/11495#issuecomment-6258969964
Thanks for the update on 9c047310570519504bd5d37be03421b46b377dba. Before I can mark the earlier points resolved I'd like to confirm them against the diff itself rather than the review summary, so a few pointers would help: - **Stale task-group contexts (F1)** – could you point me to where the restore path now instructs the surviving workers to drop the failed attempt's task-group contexts, so they stop re-reporting them? - **Clear vs. periodic report race (F2)** – as I read it the stale-report window still exists and is documented rather than closed. Documenting the race doesn't resolve a HIGH bug on its own, so can you explain how the retry is prevented from being seeded with prior-attempt counters if a report lands between the clear and the new deploy, or why that window is acceptable? On placement relative to `reset()`: both locations behave the same today, and I'm fine with it staying next to the other restore-path steps. - **Counters restarting from zero (F3)** – please point me to the incompatible-change / docs note so I can verify the wording. - **Tests (F4)** – which tests cover the cleanup-before-removal order, scope isolation, and the cleanup-failure path? Do any exercise the real metrics store rather than only verifying a mock invocation? The low-severity style notes (F5–F7) are non-blocking and can go to a follow-up if you prefer. Once the above is confirmed I'm happy to approve. <!-- streview-comment:1873 --> -- 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]
