DanielLeens commented on PR #12152: URL: https://github.com/apache/seatunnel/pull/12152#issuecomment-5713031771
@SEZ9 I can answer that directly since I traced the same test file for my Sep 16 review against this same head (`44f762eacfc6`). (a) Yes, covered. `testCheckpointErrorReportDoesNotRunOnCallerThread` (`CheckpointCoordinatorTest.java:1028-1056`) now positively asserts delivery: after `releaseExecutor.countDown()` at line 1049, it does `Mockito.verify(checkpointManager, Mockito.timeout(5000)).handleCheckpointError(Mockito.eq(1), Mockito.eq(false))` at lines 1050-1051, not just `verifyNoInteractions` before release as in the earlier round. (b) Partially. Both `testCheckpointErrorReportCompletesFutureWhenExecutorRejects` (`:1066-1077`) and `testCheckpointErrorReportRejectionStillReachesCancellationOffCallerThread` (`:1084-1110`) build the coordinator with an already-`shutdownNow()`-ed primary `executorService`, so they exercise "primary executor rejected -> fallback executor absorbs the work": the first asserts `waitCheckpointCoordinatorComplete().get(...)` returns (future completes), the second asserts `handleCheckpointError(1, false)` still runs, on a thread different from the caller's (`:1103-1109`). So "primary rejected -> handler still runs on the fallback" is covered by both. What neither test covers is the double-rejection branch: both the primary `executorService` and the static `ERROR_REPORT_FALLBACK_EXECUTOR` rejecting, which is the only path that reaches the direct `checkpointCoordinatorFuture` completion at `CheckpointCoordinator.java:585-604` without `handleCoordinatorError` running at all. Neither test shuts the fallback executor down (it is a `private static final` field with no way to inject a substitute), so that branch is exercised by neither test. I flagged this as Issue 1 (Medium, non-blocking) in my Sep 16 review. It is also currently unreachable in production, since nothing in this diff or the wider codebase ever shuts `ERROR_REPORT_FALLBACK_EXECUTOR` down, so I don't think it should hold up this round, but a fast follow-up test would be good to actually verify that branch rather than leave it present-but-unproven. -- 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]
