DanielLeens commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5846262608
Thanks @SEZ9 — you can skip that re-confirmation, I already ran it. My review on `0a4b61553` earlier (Sep 24) was a full from-scratch re-check of exactly this: I re-traced the test bodies and the fallback dispatch ordering against the current head rather than reusing our earlier notes, and F1 and F2 are both resolved there. Concretely: - `testCheckpointErrorReportDoesNotRunOnCallerThread` positively asserts delivery via `Mockito.verify(checkpointManager, Mockito.timeout(5000)).handleCheckpointError(eq(1), eq(false))` after the busy executor is released — that's F2(a). - `testCheckpointErrorReportCompletesFutureWhenExecutorRejects` and `testCheckpointErrorReportRejectionStillReachesCancellationOffCallerThread`, both built on an already-`shutdownNow()`-ed primary executor, cover F1/F2(b): the future still un-parks a `.join()`/`.get()` waiter, and the handler still runs (off the caller thread) via the fallback dispatch. Dispatch-before-complete ordering is correct, so the `isDone()` guard stays meaningful. The only things still open are two non-blocking follow-ups I flagged that round: the double-rejection last-resort branch (`CheckpointCoordinator.java:585-604`) isn't exercised by any test and is currently unreachable in production since nothing shuts down the static fallback executor, and if it's ever reached it records the wrong exception message at line 597. Neither should hold up this PR — they're fine as a fast follow-up alongside #12342, as we already discussed. So from a code standpoint I have nothing further blocking. The one open item is CI: the `Build` check on this exact head is still the same completed run I dereferenced last round (fork run `35681392273`), with 3 failures that don't touch `CheckpointCoordinator.java` — a `unit-test` Maven Central connectivity flake, two pre-existing `engine-v2-it` races already tracked in #12316/#12313 and #12311, and a `kafka-connector-it` container-teardown network flake. The branch is now 22 commits behind `dev`, so @CryoThrust, whenever convenient, a sync plus a fresh run would let us close this out with a clean (or confirmed-unrelated) result rather than relying on the Sep 22 run. Once that's up I'm ready to approve. -- 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]
