SEZ9 commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5691569301
Thanks for the write-up on the `Build` check, @CryoThrust. I agree with your read: the failing lanes are integration suites that don't touch `CheckpointCoordinator.java` or its test, which is the only diff on this branch, and the same failure pattern is showing up on unrelated PRs. I'm treating it as environmental and won't hold it against this PR. No need to push an empty commit just to re-trigger CI. To be clear about what *is* still blocking, though: the two points from the previous review are unchanged in `551442e` (which I understand is rebase-only): 1. **Unguarded `executorService.execute(...)`** in `CheckpointCoordinator.java`. With a `SynchronousQueue` + `AbortPolicy` pool, or a pool that has already been shut down during a master switch, `execute` can throw `RejectedExecutionException`. Right now that exception escapes and the task-reported error is dropped, so the pipeline can still hang in restore, which is exactly the symptom this PR is meant to fix. Please either catch the rejection and fall back to dispatching onto an executor that `clearCoordinatorService()` cannot shut down, or, if you believe the current executor's ownership makes rejection impossible here, add a comment explaining why plus a test that pins that assumption. 2. **Test coverage** in `CheckpointCoordinatorTest.java`. The current regression test only shows the caller returns while the executor is busy. Please extend it to assert that (a) the error is actually propagated to the coordinator once the executor is released, (b) an error reported while the executor rejects work is still handled rather than lost, and (c) an error reported after the executor has been shut down is still handled. Once those two land I'll do a full re-review, and I'll take a fresh look at `Build` at the same time. <!-- streview-comment:1083 --> -- 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]
