SEZ9 commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5707379369
@CryoThrust thanks for spelling out the ordering — that's the part I wanted to see, and it's the right way round. Dispatching `handleCoordinatorError` first and only completing `checkpointCoordinatorFuture` directly if that second dispatch is also rejected keeps the `isDone()` first gate meaningful; completing the future up front would turn the handler into a no-op and skip cancellation, which is the outcome we already ruled out. Running the fallback on a shared daemon single-thread pool that `clearCoordinatorService()` cannot shut down addresses the original F1 concern. I'm happy with F1 as described in `4f5a779a1`. One thing I'd still like to confirm for F2 (test coverage): you mentioned both points landed in `4f5a779a1` and posted the test count after the sync onto `b37af3a9b`. Could you point me at (or briefly describe) the assertions that cover (a) the error actually propagating and the coordinator future completing once the busy executor is released, and (b) the rejection path — primary executor rejected → handler still runs on the fallback, and double rejection → future completed directly? From the count alone I can't tell which of those are asserted. Once that's confirmed I think this is ready. <!-- streview-comment:1103 --> -- 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]
