DanielLeens commented on PR #12043: URL: https://github.com/apache/seatunnel/pull/12043#issuecomment-6594226795
Pushed `5d771553edc` on top of `248d243d5d8` for the open points in https://github.com/apache/seatunnel/pull/12043#pullrequestreview-5728218209. **Issue 2 (losing cancel overwrites a winning FAILED).** `cancelCheckpoint()` still does its early `isDone()` return and `cleanPendingCheckpoint(PIPELINE_END)`, but the CANCELED write and completion now run inside `synchronized (restoreProgressLock)` with a second `isDone()` check. `failRestoreProgressIfCurrent` already wins the future and persists FAILED under that same lock, so whichever path completes the future is now the only path that writes the terminal status. The write uses `persistStatus` instead of `updateStatus`: holding `restoreProgressLock` while taking the coordinator monitor would reverse the documented lock order (`completePendingCheckpoint` takes the monitor and then this lock). Regression test: `CheckpointRestoreProgressConcurrencyTest#cancelLosingToFailFastDoesNotOverwriteFailedStatus`. It stubs the spied `cleanPendingCheckpoint(PIPELINE_END)` so the fail-fast timeout runs after cancel has passed its first guard, then asserts that the persisted status and the returned future are both FAILED. On the old code the persisted status becomes CANCELED, so the test fails. What this does not cover: the existing dev-side terminal paths (`handleCoordinatorError` and the SUSPEND/FINISHED completion in `completePendingCheckpoint`) still write status and then complete the future outside this lock. This PR leaves that pattern as it is. **Issue 3 (lifecycle contract).** Added Javadoc for `restoreProgressLock` (lock order, and why `persistStatus` is used under it), `restoreProgressGeneration` (generation fencing), `startRestoreProgressTracking` / `markAllTasksReadyAfterRestore` / `markPostRestoreCheckpointProgress` (the two phases and when each ends), `handleRestoreProgressTimeout` (warning vs. fail-fast routing) and terminal ownership on `failRestoreProgressIfCurrent`. `SubPlan#handleRestoreProgressTimeout` already documents its monitor contract. **Still open:** - Issue 4 (extend the existing failover engine IT with a restored-job progress case and a controlled fail-fast stall) is not in this push. To stall checkpoint progress deterministically in a real restored cluster, the test needs a controllable source or sink, and I want to design that properly instead of adding a timing-dependent IT. The PR stays draft until that lands. - Issue 5 (non-blocking, real 1s timer in `CheckpointCoordinatorTest`) is unchanged. Only `spotless:apply` was run locally. The fork CI on the new head is the verification, and I am not claiming a pass until it 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]
