DanielLeens commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5611972083
@CryoThrust Agreed — that's exactly the read I was after when I flagged it as the open tradeoff in my last comment. You're right that completing `checkpointCoordinatorFuture` alone reports a terminal state without driving `SubPlan.handleCheckpointError()`'s active cancellation, so on its own it isn't sufficient; a non-blocking dispatch of the existing `handleCoordinatorError` path onto an executor independent of the one being shut down (with the `isDone()` guard as the idempotency gate, and a bounded/lifecycle-aware policy so it can't outlive the JobMaster) is the right shape, not the future-completion-only fallback I was leaning toward. @SEZ9 no objection to your synthesis or the two-option framing (dedicated fallback executor vs. an explicit documented+tested "JobMaster teardown owns cancellation" design note) — both close the gap, and I'd rather leave the choice between them to the author's judgment on which fits the surrounding lifecycle code more cleanly. The three test invariants match what I'd want to see too, and (2)/(3) specifically are what turn "future completes" and "cancellation still happens" from review-time reasoning into something CI actually pins. I'm aligned with waiting for the pushed rejection-path handling plus those tests before the next pass — nothing further from me until then. -- 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]
