DanielLeens commented on PR #12134: URL: https://github.com/apache/seatunnel/pull/12134#issuecomment-6607599042
Thanks @SEZ9, I appreciate the careful pass. Going point by point on `c6fd7fc0d575c9c316fb41beb94eb1fd6dcaefa0`: **CI (the real remaining gap): now green, and the failover IT actually executed.** The cancelled jobs of fork run [41162516391](https://github.com/DanielLeens/seatunnel/actions/runs/41162516391) were re-run, and attempt 2 is `completed` / `success` on this exact head (83 success, 13 skipped, 0 failed). `CheckpointCoordinatorFailoverIT` ran in both engine lanes with `Tests run: 6, Failures: 0, Errors: 0`, including `testStreamJobResolvesToSavepointDoneAfterMasterFailoverDuringSavepointShutdown`: - [engine-v2-it (11, ubuntu-latest)](https://github.com/DanielLeens/seatunnel/actions/runs/41162516391/job/125695845200) - [engine-v2-it (17, ubuntu-latest)](https://github.com/DanielLeens/seatunnel/actions/runs/41162516391/job/125695846277) The Apache-side check list for this SHA is still empty; that is the same check-adoption gap as before, not a test result. **F1 (PR description).** Agreed. I will add both documented limits (the deadline is only checked between attempts, and the redrive intent is heap-only, so a second failover before the redriven savepoint re-persists `DOING_SAVEPOINT` drops it) to the PR description, and list persisting the redrive intent as a follow-up. **F2 / F5 (missed-window message).** A missed window already fails with an explicit message at three of the four checkpoints: `CheckpointCoordinatorFailoverIT.java:1145` ("savepoint checkpoint completed on the old master before shutdown"), `:1191` (the client call returned normally, "which means masterNode1 completed the savepoint before it was shut down and the post-failover redrive was never exercised"), and `:1211-1221` (the ending savepoint "was completed by the old master and not redriven by the restored master"). The weak spot is the shared guard `assertSavepointCallStillPending` (`:1386-1396`): when the call has already returned it only says "savePointJob completed before master failover". You are right that this should name the cause; I will reword it to say the savepoint finished on the old master before failover and the redrive path was not exercised, so a flake is triaged as a test-timing miss rather than a product failure. **Repeated runs.** I do not have a recorded tally of how often the window is missed on this head, and I would rather not quote an unrecorded number. (The ~10% in the description refers to a different, pre-existing master-switch restore race seen during the investigation, not this window.) After the next push I will re-run the two `engine-v2-it` lanes on the fork several times and post the pass/fail count here. **F3 / F6.** My previous comment was not truncated in the stored body; restating the conclusion: the reflection into `pendingCheckpoints` stays. `CheckpointCoordinator` has no public accessor for that map, and `PendingCheckpoint#isFullyAcknowledged()` is `protected`, so a test-visible hook would mean a production-code change for a test-only need. The reason is already written down in the test: the helper Javadoc at `:1398-1409` and the test-method Javadoc at `:993-1006`, both noting that `CheckpointCoordinatorTest` uses the same idiom. `ignoreExceptions()` is at `:1120`, with the reason it is safe (the verdict on a finished call is taken on the test thread) at `:1110-1115`. **F4 / F7 / F8.** Confirmed on this head: - `savepointCaller.shutdownNow()` is the first statement of the `finally` block (`:1223`), so it runs on every exit path. - The call outcome is checked before failover by `assertSavepointCallStillPending` (`:1129`, `:1138`), which rethrows a recorded failure as `AssertionError("savePointJob failed ...", cause)` with the original cause attached (`:1393`). - After `SAVEPOINT_DONE`, `savepointCall.get(30, TimeUnit.SECONDS)` plus the recorded-failure assertion (`:1188-1194`) inspect the result, so a pre-failover `savePointJob` failure surfaces as itself, not as the 30 s Awaitility timeout. Next push: the description update and the clearer guard message, followed by the repeated fork runs. Thanks again for keeping this test honest. -- 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]
