SEZ9 commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5611769852
@DanielLeens — thanks for checking the raw body; agreed, that was the "show more" collapse and not truncation, so that question is closed. And thanks for digging one level further on the `.join()` point. I agree with your conclusion: because `checkpointCoordinatorFuture` is consumed via plain `CompletableFuture.join()` semantics, the interrupt from `shutdownNow()` does not release a parked waiter, and letting the `RejectedExecutionException` escape to the operation thread does not complete that future for anyone. So "visible operation failure as the fallback" is not an acceptable resolution for the rejection case — it just moves where the hang shows up. I'm withdrawing that suggestion. @CryoThrust — your downstream trace matches how I read it as well. Completing the future is the necessary minimum to un-park `join()` callers, but on its own it doesn't drive the pipeline to `CANCELING` through `SubPlan.handleCheckpointError()`, so we'd risk reporting a terminal state while leaving task cancellation to whatever happens to tear things down later. I'm fine with the direction you outlined for this PR: 1. On rejection: keep the `checkpointCoordinatorFuture.isDone()` guard as the first gate, log at WARN with the pipeline/master-lifecycle context, complete the future as the last-resort signal, and dispatch the existing `handleCoordinatorError` path non-blockingly on an executor that is not the one being shut down, with a bounded/lifecycle-aware policy so it can't keep the JVM alive after the JobMaster is gone. 2. If you'd rather not introduce a second executor, the alternative is acceptable only with the explicit design note that cancellation is owned by JobMaster teardown, plus a regression test that actually proves that ownership — the current code doesn't establish it. For the tests, the three invariants you listed are exactly what I'd want to see and they close the remaining gap in my earlier test finding: (1) the normal executor path propagates the error and cancels the pipeline once the executor is released (not just "the caller returned"), (2) a rejected submission completes the future so a `join()` waiter cannot stay parked, and (3) the rejected path still reaches cancellation (or verifies the documented alternate owner) without running the full state machine on the operation thread. Concrete asks before I re-review: pick option 1 or 2 above and say which in the PR description, push the rejection-path handling, and add the three tests. Ping me when that's up and I'll take another pass. <!-- streview-comment:932 --> -- 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]
