SEZ9 commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5594877719
Thanks @DanielLeens — that's a thorough trace against `f3fcb611e4`, and I agree with it. **On the operation-thread concern:** agreed, that settles it. Since `CheckpointErrorReportOperation.runInternal()` is the only caller and it invokes `reportCheckpointErrorFromTask()` synchronously on the Hazelcast operation thread, a naive catch of `RejectedExecutionException` that falls back to inline `handleCoordinatorError(...)` would reintroduce exactly the blocking work this PR is moving off that thread. So I'm withdrawing the inline fallback as an acceptable shape for F1. **On the `CompletableFuture.runAsync(...).exceptionally(...)` point:** you're right, and I should have caught that when I suggested mirroring `reportedTask()` — a rejected submission never reaches the returned future. Your comment appears to be cut off mid-sentence at that point, though; if you had a further point or a proposed shape after it, please post the rest. Given both of the above, here is where I land on the remaining asks: 1. **F1 (rejection fallback)** — still blocking. The fallback needs to (a) not silently drop the report and (b) not do checkpoint-manager work on the operation thread. My current thinking is that the two rejection causes may want different handling: after `shutdownNow()` from `clearCoordinatorService()` this node is no longer active master and the coordinator is being torn down anyway, so logging and letting the exception escape `runInternal()` (so the operation fails visibly instead of returning success) may be sufficient; for the saturation case on the `SynchronousQueue`/`AbortPolicy` pool, silent drop is not acceptable but neither is inline processing. I'd like your view on whether a visible failure of the operation is enough given the senders discard the `sendToMaster` future, or whether you see a cheap non-blocking way to still get the pipeline out of `waitCheckpointCoordinatorComplete(...).join()` in that window. I'd rather agree on the shape before code lands. 2. **F2 (test coverage)** — two additions: release the latch in `testCheckpointErrorReportDoesNotRunOnCallerThread` and positively assert `checkpointManager` receives the error (rather than only `verifyNoInteractions` before release), plus a new case with an already-shutdown/rejecting executor that asserts whatever fallback behavior we agree on in (1). Once the fallback shape is settled I'll do the full re-review against the updated head. Thanks again for the careful checking. <!-- streview-comment:908 --> -- 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]
