SEZ9 commented on PR #12134: URL: https://github.com/apache/seatunnel/pull/12134#issuecomment-6051770224
Thanks for the explanation — the commit message in `872701ec8` plus the new PR description section makes the widened assertion clear, and one green fork run (37388052981 on `38c06bbd501`) is fine as a sanity check rather than a flake-rate claim. I won't block on that. What is still open from the earlier review of `CheckpointCoordinatorFailoverIT.java`: 1. **Production fix not reviewable.** The `CoordinatorService` / `JobMaster` changes are what make `SAVEPOINT_DONE` after failover possible, but they are not visible in the diff body. Could you make sure the full diff renders, or summarize the redrive logic in the PR description, so the test can be judged against what it guards? 2. **Race between the in-flight savepoint and the master kill.** As written, the savepoint can complete on the old master before `masterNode1.shutdown()` lands, and the test still passes without touching the redrive path. Please add a check that the savepoint was still pending at shutdown and/or that completion happened on the new master, or fail the test if it degraded into the "completed on old master" scenario. 3. **Reflection polling can abort the wait.** Reading the private `pendingCheckpoints` field via reflection mid-transition can throw and kill the Awaitility loop. At minimum add `ignoreExceptions()`; ideally replace the private-field probe with a test-visible hook so the regression guard isn't coupled to the coordinator internals this PR is changing. 4. **Executor and future lifecycle.** `savepointCaller` is created outside the try block and its shutdown isn't visible; `savepointCall` is never inspected. Please shut the executor down on every exit path and check the future's outcome so a pre-failover savepoint failure fails fast instead of surfacing as an unrelated 30s timeout. Happy to take another pass once those are addressed. <!-- streview-comment:1599 --> -- 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]
