SEZ9 commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5578100091
Thanks @DanielLeens — appreciate you re-checking the master-switch shutdown path and the executor's queue/rejection shape. That lines up with my read, so the two open points remain: 1. **Rejection fallback** — `executorService.execute(...)` in `reportCheckpointErrorFromTask()` still has no guard. Since the senders of `CheckpointErrorReportOperation` discard the `sendToMaster` future, a `RejectedExecutionException` (pool saturated, or shut down on master switch) would drop the report and leave the pipeline waiting on the coordinator future — the same hang this PR targets. Please either catch `RejectedExecutionException` around `execute(...)` and fall back to running `handleCoordinatorError` inline, or mirror the `CompletableFuture.runAsync(...).exceptionally(...)` shape that `reportedTask()` already uses so the failure is at least logged and handled. Either is fine; a short note on the choice in the PR description would help. 2. **Test coverage** — the current test only shows the caller returns while the executor is busy. Please add a positive assertion that the error is actually propagated once the executor is released, plus a case where the executor is already shut down (or saturated) at call time, asserting the report is still delivered via the fallback rather than silently dropped. If there's a reason the inline fallback is undesirable (e.g. re-entering the JobMaster state machine on the operation thread during shutdown), happy to discuss here first. I'll do a full re-review once these land. <!-- streview-comment:881 --> -- 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]
