DanielLeens commented on PR #12134: URL: https://github.com/apache/seatunnel/pull/12134#issuecomment-6466889583
Thanks @SEZ9, and sorry the production side was hard to find in the PR view. The three source files that matter are `CoordinatorService.java` and `JobMaster.java` (production) and `CheckpointCoordinatorFailoverIT.java` (test). Answers below are against head `6011de825`. **Production redrive task** (`CoordinatorService.redriveSavepointAfterMasterFailover`, around lines 1256-1320; the call site is inside the scheduling lambda around line 518, after `consumeRedriveSavepointAfterRestore()`): - Retry loop: until a deadline of `REDRIVE_SAVEPOINT_WAIT_MILLIS` (5 minutes), read `jobMaster.getPhysicalPlan().getJobStatus()`. If the status is an end state, return. If it is `RUNNING`, call `jobMaster.savePoint().get()`: `true` returns, `false` or any exception (for example `TASK_NOT_ALL_READY_WHEN_SAVEPOINT` while tasks are still redeploying) logs at INFO and retries after a 500 ms sleep. Any other status just sleeps. - Termination: it cannot spin forever. It exits on success, on a terminal job state, on interrupt, or when the 5-minute deadline passes (WARN, "client's original savepoint request will need to be retried manually"). If the job never reaches `RUNNING`, the loop only polls and then gives up at the deadline; it never touches the job. - Half-state: the task never changes job state by itself. A failed savepoint start goes through the existing `JobMaster#restoreRunningAfterSavepointStartFailure`, which puts the job back to `RUNNING`, so the worst case is the pre-fix behavior (savepoint request dropped), not a stuck `DOING_SAVEPOINT`. `jobMaster.run()` does not depend on this task, and a `RejectedExecutionException` on submit is caught and logged. - Honest limits (documented in the Javadoc): the deadline is only checked between attempts, so one `savePoint().get()` is bounded by the checkpoint timeout rather than by the 5-minute budget; the task holds one shared-executor thread for up to 5 minutes per redriven job; and a second failover before the redrive succeeds loses the request again, because the redrive intent is heap-only. **E2E hardening in `419000d09ee`:** - F2 / F5: before `masterNode1.shutdown()` the test asserts `savepointCall` is still not done and re-evaluates `isSavepointGenuinelyMidShutdown(...)` (job status is `DOING_SAVEPOINT` and a savepoint `PendingCheckpoint` exists whose `getCompletableFuture()` is not done). A savepoint that completed on the old master therefore fails the test with "savepoint checkpoint completed on the old master before shutdown" rather than producing a false pass. The final assertion is `SAVEPOINT_DONE` read through the client after failover. - F3: yes, the poll uses `.ignoreExceptions()` with a 10 ms interval, so a transient failure while the master's state is changing does not abort the wait. - F4 / F7 / F8: `savepointCaller.shutdownNow()` is in the `finally`, followed by closing the client and shutting down both masters. The future's outcome is inspected in two places: inside the pre-failover poll (a completed or failed `savepointCall` fails the wait immediately, with the cause attached) and with `savepointCall.get(30, SECONDS)` at the end. After `masterShutdownStarted` is set, an exception from the client call is expected and only logged. - F6: it still reflects into the private `pendingCheckpoints` map via `ReflectionUtils.getField`, because there is no public accessor. The Javadoc on `hasUnacknowledgedSavepointCheckpoint` explains the coupling and why `isFullyAcknowledged()` is not used. If you prefer, I can follow up with a small package-visible accessor in a separate change, but I did not want to widen the production diff here. I'm happy to add anything else you want to see before moving toward approval. -- 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]
