CryoThrust commented on PR #12149: URL: https://github.com/apache/seatunnel/pull/12149#issuecomment-5694320987
@DanielLeens correction to my previous comment first: I described the two non-blocking items as "the `NOT_STARTED_STATUSES` Javadoc nit and the pre-existing race" — that was #12156's list, not this PR's. Yours here were the `createScheduler()` comment (Issue 1) and the pre-lock `RejectedExecutionException` window (Issue 2). Both are now done in `6741ec1fe`, along with the optional assertion. **Issue 1** — `createScheduler()` now carries a comment saying it builds a live 2-thread scheduler and that only a `CHECKPOINT_COORDINATOR_RESET` cleanup may replace a shut-down one, so the RESET-only invariant is visible to a reader who opens that method alone. **Issue 2 — option A.** I guarded the underlying `scheduler.schedule(...)` in `scheduleTriggerPendingCheckpoint` rather than the individual call sites, so both the pre-lock and the in-lock callers are covered by one guard. On rejection it logs and skips the reschedule, with the reasoning in a comment: the only way to reach it is losing the race to a terminal cleanup, and the coordinator is ending anyway. **Optional assertion** — `testResetRecreatesLiveScheduler` calls `restoreCoordinator(true)` and asserts the scheduler is *not* shut down, so the RESET half of the invariant is explicitly tested rather than incidentally covered by the existing double-`restoreCoordinator` tests. Synced head is now `6741ec1fe` on top of `b37af3a9b`: ``` CheckpointCoordinatorTest 18 run, 0 failures ``` Ping me if the synced CI still shows anything and I'll dig in. -- 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]
