DanielLeens commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-6116670029
Thanks @SEZ9, and thanks for re-checking this against the rebased head. I went through `de042f22b` again to answer your three F2 points and the comment ask directly, so nothing is left ambiguous. **Comment on the last-resort branch.** It is already there. In `CheckpointCoordinator.reportCheckpointErrorFromTask()`, the inner `catch (RejectedExecutionException fallbackFailure)` carries a comment explaining that nothing could run the handler, so the future is completed as a last resort: it cannot drive cancellation itself, but it releases waiters instead of leaving them parked, and cancellation is then owned by whatever tears the JobMaster down for the failed coordinator state. No change needed there. **F2 test mapping (all in `CheckpointCoordinatorTest`):** 1. Propagation after the busy executor is released: `testCheckpointErrorReportDoesNotRunOnCallerThread` asserts `verifyNoInteractions(checkpointManager)` while the executor is busy, then after release verifies `handleCheckpointError(1, false)` with a 5s timeout. So the report is actually handled, not just accepted. 2. Shut-down primary executor (master-switch scenario): `testCheckpointErrorReportRejectionStillReachesCancellationOffCallerThread` uses a `shutdownNow()`-ed executor, verifies `handleCheckpointError` is reached via the fallback executor, and asserts the handler thread differs from the caller thread. `testCheckpointErrorReportCompletesFutureWhenExecutorRejects` covers the waiter-release side with the same setup. 3. Both executors rejecting: this one is not covered. The fallback executor is a private static daemon pool, so a test cannot make it reject without reflection or a seam, and the branch is unreachable today. The author already agreed to track it alongside #12342, and I would keep it as a non-blocking follow-up rather than add reflection-based tests here. So from my side the F1/F2 items are resolved on this diff and there is no code-side blocker. Remaining ask is only a fresh CI run on the current head; the failures reported so far were not tied to `CheckpointCoordinator`, so a re-run of the failed jobs is the right next step if they recur. -- 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]
