SEZ9 commented on PR #11489: URL: https://github.com/apache/seatunnel/pull/11489#issuecomment-5366241903
Thanks @DanielLeens for the thorough from-scratch re-review of `2b840e976`, and for confirming the retry-test ordering issue from your `037ea4335` review is now resolved. To recap on that single point: the problem was that the retry case faked the checkpoint coordinator as ready immediately after the task entered RUNNING, so the second savepoint request could start before the real task-ready handshake completed, eventually failing the job on the 60s checkpoint timeout. The fix in `2b840e976` follows exactly the approach you suggested — `SavePointBusySourceTest.java` now calls `awaitCheckpointCoordinatorsReady(jobMaster)` to wait for the real coordinator to be ready before each `setCheckpointCoordinatorsReady(jobMaster, false)` injection, so the precondition failure is injected deterministically rather than racing the handshake. Glad your verification confirms both `setCheckpointCoordinatorsReady(jobMaster, false)` call sites are now guarded and that nothing else moved between `037ea43357f03827622196d26f85e1bff2680568` and `2b840e97663480e10bef169ac963a0ae6580dccb` (`+2/-27`, with the `TaskExecutionService.java` change removed outright). Given both of your approvals on the current head, I have no remaining code asks on this point. Only two housekeeping items before merge: please confirm the APPROVED state stands on `2b840e976` as the final head, and flag here if you'd like the two-commit history squashed on merge or kept as-is. Otherwise I'll proceed once you confirm. Thanks again for the careful, byte-level re-verification — it made this round easy to close out. <!-- streview-comment:419 --> -- 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]
