DanielLeens commented on PR #12134: URL: https://github.com/apache/seatunnel/pull/12134#issuecomment-6592034076
@SEZ9 thanks for the status check on `c6fd7fc0d575c9c316fb41beb94eb1fd6dcaefa0`. I re-read the current head against each point. Most of them are already in the code on this head; the one real gap is CI evidence. **F1 - production side.** `CoordinatorService.java` (+143) and `JobMaster.java` (+52) are part of this PR's Files changed; if the diff view collapsed them for you, that is a rendering issue, not a missing change. What the new master does with an inherited pending savepoint is redrive, not abort-and-restart: - `restoreJobFromMasterActiveSwitch` reads the persisted pre-restore state and calls `JobMaster#markRedriveSavepointAfterRestore()` only if it was `DOING_SAVEPOINT`. - The scheduling lambda calls `consumeRedriveSavepointAfterRestore()` (consume-once) and submits `redriveSavepointAfterMasterFailover`, which runs concurrently with the blocking `jobMaster.run()`. - The redrive polls every 500 ms. It exits on a terminal job status, a successful `savePoint()`, interruption, or the 5-minute `REDRIVE_SAVEPOINT_WAIT_MILLIS` deadline, after which it logs a warning and leaves the job as it was. A rejected submission is logged and does not block job startup. - It goes through the normal `JobMaster#savePoint()` entry, so failure handling stays in the existing JobMaster/PhysicalPlan paths; the redrive adds no new job state. Two limits that are real and documented in the Javadoc: the deadline is only checked between attempts, so one blocking `savePoint().get()` can outlast the nominal 5 minutes; and the redrive intent is heap-only, so a second failover before the redriven savepoint re-persists `DOING_SAVEPOINT` drops the request again. Persisting the intent would be a separate follow-up. **F2 / F5 - proof that failover was exercised.** This is what `c6fd7fc0d` adds: the client's `savePointJob` call is pinned to the old master's member uuid, so the test asserts that call failed (a normal return would mean the old master finished it), and it asserts the completed savepoint's trigger timestamp is strictly greater than the in-flight one captured just before `masterNode1.shutdown()`. Checkpoint ID cannot discriminate because the counter is re-seeded on restore. The observe-then-shutdown window is still check-then-act, so a missed window now fails loudly instead of passing, but it is not deterministic; I recorded that as residual timing risk, not as solved. **F3 / F6.** `ignoreExceptions()` on the in-flight poll landed in `419000d09`, with a comment on why the verdict for a finished call is taken on the test thread (it would swallow `AssertionError`). The test still reflects into the private `pendingCheckpoints` field: there is no public accessor, and the Javadoc explains the coupling and that `CheckpointCoordinatorTest` uses the same idiom. If you know of a test-visible accessor I missed, I will switch to it; adding one would mean touching production code, which I did not want to do for a test-only need. **F4 / F7 / F8.** `savepointCaller.shutdownNow()` is in the `finally` block, and `savepointCall` is checked via `assertSavepointCallStillPending` before the shutdown and `savepointCall.get(30s)` plus the recorded-failure assertion after `SAVEPOINT_DONE`, so a pre-failover `savePointJob` failure surfaces as itself. These test changes in `c6fd7fc0d` are the follow-up I owed after your earlier round. **CI.** Agreed, there is no executed evidence on this head. Fork run 41162516391 is still `cancelled` and I do not see a newer attempt. I will re-run only the cancelled jobs and link the result here; I am not claiming a pass until that completes. -- 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]
