DanielLeens commented on PR #12134: URL: https://github.com/apache/seatunnel/pull/12134#issuecomment-6061169904
Thanks @SEZ9, and thanks for the green-run acknowledgement. Going through the four open points against head `38c06bbd501`: 1. **Production fix not visible in the diff.** The full diff does include the fix: the PR changes `CoordinatorService.java` and `JobMaster.java` under `seatunnel-engine-server/src/main`, in addition to the IT and its conf template. If the rendered view collapsed them for you, `gh pr diff 12134` lists all four files. I'll also add a short redrive summary to the PR description so the test can be read against it. 2. **Old-master completion race.** You are right. The poll only proves the savepoint checkpoint was pending at observation time; it can still finish on `masterNode1` before `shutdown()` lands, and the test would pass without exercising the redrive. I'll add a guard so the test fails if the savepoint completed before the kill. 3. **Reflection probe inside Awaitility.** Agreed. `hasUnacknowledgedSavepointCheckpoint` reads the private `pendingCheckpoints` field and a mid-transition exception would abort the wait. I'll add `ignoreExceptions()` and look at whether a test-visible hook is cheap enough here. 4. **Executor and future lifecycle.** `shutdownNow()` is in the `finally`, but the executor is created before the `try`, and `savepointCall` only gets `get(30s)` after the lambda has already swallowed any exception, so a pre-failover failure would not surface. I'll move creation inside the `try` and record the failure outcome so it fails fast. I have not pushed these yet, so there is no new head to review. I'll push one commit covering 2-4 and update here when it is up. -- 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]
