DanielLeens commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5569386633
Thanks for digging into this, @SEZ9 — you're right, and this changes my merge recommendation. I had flagged the RejectedExecutionException risk in my own review (section 1.4) but concluded it wasn't reachable because I "did not find a code path that tears this executor down while such reports can still arrive." That conclusion was wrong. I went back and checked: `CoordinatorService.clearCoordinatorService()` calls `executorService.shutdownNow()` (CoordinatorService.java:1312), and that runs from `checkNewActiveMaster()` the moment this node steps down as active master (CoordinatorService.java:1246-1253) — exactly the master-switch window where an in-flight `CheckpointErrorReportOperation` from a worker can race the shutdown. Since every sender of that operation (`NotifyTaskRestoreOperation`, `BarrierFlowOperation`, `SourceRegisterOperation`, `CheckpointBarrierTriggerOperation`) discards the `sendToMaster` future, a `RejectedExecutionException` out of `execute()` at `CheckpointCoordinator.java:522` would silently drop the report and leave `SubPlan` parked on `waitCheckpoi ntCoordinatorComplete(...).join()` forever — reproducing the exact hang this PR (and #12139) is meant to fix, just via a different trigger. I also confirmed `createCoordinatorExecutor()` (CoordinatorService.java:287-297) is a `ThreadPoolExecutor` over a bare `SynchronousQueue` with the `AbortPolicy`-derived `RejectionCountingHandler`, so this is exactly the pool shape that throws on saturation or after shutdown, not a theoretical corner case. Agreed on both remaining asks: catch `RejectedExecutionException` and fall back to inline `handleCoordinatorError` (or mirror `reportedTask()`'s `CompletableFuture.runAsync(...).exceptionally(...)` shape), and close the test gap — the current test only proves the call is asynchronous, not that the error is actually delivered once the executor is free or shut down. Revising my conclusion from "ready to merge" to blocking pending the rejection-fallback fix plus a positive assertion (and a shutdown/rejection test case). Will do a full re-review once that lands. -- 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]
