SEZ9 commented on PR #12134: URL: https://github.com/apache/seatunnel/pull/12134#issuecomment-6594350523
Thanks for the detailed walkthrough against `c6fd7fc0d575c9c316fb41beb94eb1fd6dcaefa0`. **F1 (production side)** — The redrive design as you describe it (`markRedriveSavepointAfterRestore()` only on `DOING_SAVEPOINT`, consume-once `consumeRedriveSavepointAfterRestore()`, 500 ms poll, 5-minute `REDRIVE_SAVEPOINT_WAIT_MILLIS`, routed through the normal `JobMaster#savePoint()` with no new job state) is the right shape. I'll re-read the `CoordinatorService.java` / `JobMaster.java` changes on the current head; if they were collapsed in the diff view, that's on my side. The two documented limits are acceptable for this PR as long as they are also stated in the PR description, not only in the Javadoc — please add them there, and note that persisting the redrive intent is a follow-up. **F2 / F5 (proof failover was exercised)** — Pinning `savePointJob` to the old master's member uuid and asserting that the call fails and that the completed savepoint's trigger timestamp is strictly greater than the in-flight one captured before `masterNode1.shutdown()` is a good discriminator, and I agree checkpoint ID can't do it given the re-seed on restore. Failing loudly on a missed window is the right trade-off. Two asks: make sure the failure message for the missed window clearly says the savepoint completed on the old master (so a flake is triaged correctly), and please share results of running the IT several times locally so we have a feel for how often that window is missed. **F3 / F6** — Thanks for pointing to `419000d09` for `ignoreExceptions()`; I'll verify it on the current head. Your reply appears to have been cut off under this heading — could you repost your conclusion on F6 (reflection into private `pendingCheckpoints` across modules)? If the reflection stays, a short note in the test on why there's no public hook is enough for me. **F4 / F7 / F8** — Could you confirm on the current head that `savepointCaller` is always shut down and that `savepointCall`'s outcome is inspected, so a pre-failover savepoint failure surfaces directly rather than as the 30 s Awaitility timeout? **CI** — Agreed this is the real remaining gap. Please point me at a run on the current head where the failover IT actually executed and passed; I'll do the final pass once that's up along with the F4/F6/F7/F8 confirmations. <!-- streview-comment:2247 --> -- 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]
