CryoThrust commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5724708473
@DanielLeens thanks for tracing that against the live head — I independently confirm both halves of your reading, and it's the same picture I get from the test bodies. On (a): `testCheckpointErrorReportDoesNotRunOnCallerThread` releases the busy executor and then does `Mockito.verify(checkpointManager, Mockito.timeout(5000)).handleCheckpointError(eq(1), eq(false))`, so the report is asserted *delivered*, not just accepted. Agreed that's the F2(a) assertion @SEZ9 was asking about. On (b): both rejection tests build the coordinator with an already-`shutdownNow()`-ed primary executor, so both exercise "primary rejects → fallback absorbs"; one asserts the future completes, the other asserts the handler ran on a different thread. Agreed. And agreed on what's left: the double-rejection branch — the only path reaching the direct `checkpointCoordinatorFuture.complete(...)` at `CheckpointCoordinator.java:597-606` with `handleCoordinatorError` never running — is exercised by neither test. Your framing that it's currently unreachable in production is also right: nothing in this diff or the wider codebase shuts `ERROR_REPORT_FALLBACK_EXECUTOR` down. One thing I checked rather than assumed, since it decides how a follow-up test would even be written: the pool is `private static final`, and on the JDK 11 this branch builds with, reflection **cannot** replace a static final field — `Field.set` throws `IllegalAccessException: Can not set static final ... field`. (Instance final fields are fine; static final is the one the VM blocks by default. I verified this with a throwaway class rather than trusting memory.) So writing that test needs a seam, not just a Mockito mock — something like extracting the last-resort completion into a `@VisibleForTesting` method, which would fit the five existing `@VisibleForTesting` members on this class. I'm deliberately *not* pushing that into this PR: it would reopen a diff you and @SEZ9 have already reviewed twice, and the branch can't be reached today. If you'd both rather have it proven now than tracked, say so and I'll add the seam as a new commit; otherwise I'll file it as a small follow-up alongside #12342 and keep this PR at `44f762eac` so the approval stands. -- 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]
