SEZ9 commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-6105470199
Thanks for the rebases and for coming back to correct the CI report, @CryoThrust — that kind of candour is appreciated. **Diff state.** I compared the changed lines in `CheckpointCoordinator.java` and `CheckpointCoordinatorTest.java` on `de042f22b` against `0a4b61553`: they are identical, so this is a pure rebase and my earlier review stands. **F1 (rejection path).** The current shape — `executorService.execute(reportError)`, falling back to the static daemon `ERROR_REPORT_FALLBACK_EXECUTOR` on `RejectedExecutionException`, and only completing the coordinator future directly if that also rejects — addresses the concern that a rejected report could be silently dropped and leave `join()` waiters hanging. I'm good with this part. One small ask: please add a short comment next to the last-resort branch explaining why completing the future there is acceptable (the report is not re-queued), so the next reader does not mistake it for a leftover. **F2 (test coverage).** This is the remaining open item. You mentioned `CheckpointCoordinatorTest` is 19/19, but from the thread I can't tell what the new cases assert. Could you confirm (or add) the following: 1. After the busy executor is released, the test asserts the error actually propagates — i.e. `handleCoordinatorError` runs and the coordinator ends in the expected failed/cancelled state, not just that the caller returned. 2. A case where `executorService` has been shut down (the master-switch scenario) that verifies the fallback executor picks the report up. 3. A case where both executors reject, verifying the coordinator future is completed so waiters are released. If some of these are already there, just point me at the test method names and I'll re-check. **CI.** Thanks for the honest breakdown of run `37615433319` (70 successful, 11 failed, 13 skipped). Having gone through the failed jobs, they look like runner-side problems rather than test results, and nothing in them references this diff — so I'm not treating them as blockers for this PR. If the next run is still red on the same jobs, a re-run should be enough. Once the F2 tests are in place I think this is ready. <!-- streview-comment:1680 --> -- 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]
