CryoThrust commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5693748887
@SEZ9 both points are in `4f5a779a1`, and the PR description now states the choice explicitly. **F1 — option 1, with one ordering detail.** A rejected report is dispatched on a shared, daemon, single-thread pool that `clearCoordinatorService()` cannot shut down, so the handler still runs and still runs off the operation thread. If that dispatch is rejected too, the coordinator future is completed directly as a last resort so a parked `join()` is released, with cancellation left to the JobMaster teardown of the failed coordinator state. The ordering is the part worth a second look, because getting it backwards silently defeats the fix: the fallback dispatches `handleCoordinatorError` **first** and only completes the future directly if *that* dispatch is rejected. `handleCoordinatorError` opens with the `checkpointCoordinatorFuture.isDone()` guard you asked me to keep as the first gate — so completing the future up front would turn the whole handler into a no-op and skip cancellation, which is the "reports FAILED while cancellation never happens" outcome you already ruled out. The guard stays effective, and the second rejection path is the only place the future is completed without the handler running. Daemon threads on the fallback pool are what keeps it from holding the JVM open after the JobMaster is gone; it is shared and single-threaded because rejection is exceptional, so it cannot grow per coordinator. **F2 — three tests, all three invariants.** As listed in the description: the normal path now positively asserts the error reaches `checkpointManager.handleCheckpointError(1, false)` after release (not just "the caller returned"); a shut-down executor still completes the future so `waitCheckpointCoordinatorComplete().get(...)` returns; and the rejected path reaches cancellation with the handler thread asserted to differ from the caller's, which is what pins "not on the operation thread" rather than just asserting the observable outcome. Reverting only `reportCheckpointErrorFromTask` fails exactly (2) and (3) with `RejectedExecutionException`, i.e. the dropped-report symptom. `CheckpointCoordinatorTest`: 18 run, 0 failures. Spotless clean. Ready for the re-review. Separately, the F2 follow-up from #12156 (the check-then-act race in `cancelJob()`/`stopJob()` against the `PhysicalPlan` monitor) is now tracked as #12342 — different PR, but I am dropping the link here since the two threads kept being read together. -- 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]
