Vivek1106-04 commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-5756762807
@DanielLeens Pushed `31a454f35`. One commit, and a report on Issue 1 that does not close it. ### What landed `[Fix][Zeta] Run the checkpoint trigger body off the registering thread` — 2 files, +74/-2. `startTriggerPendingCheckpoint` registered its body with `thenAccept`, so the body ran inline on whichever thread completed the pending checkpoint future, and on the caller itself when that future was already complete. That body blocks on `allOf(...).get()` for a whole barrier round-trip, and `tryTriggerPendingCheckpoint` holds the coordinator lock across the call. The caller is a shared dispatch thread, so one checkpoint could hold a member-wide thread, and the coordinator lock, for as long as that checkpoint took. Before the scheduler was shared this only stalled the coordinator's own timer thread; with a shared pool the blast radius is the member. Now registered on `executorService`, where the rest of the barrier work already runs. Test: `CheckpointCoordinatorTest#testTriggerBodyNeverRunsOnTheRegisteringThread` completes the pending future before registration and asserts the body does not run on the caller. It fails on the old code with `expected: not same but was: <Thread[main,5,main]>` and passes on the new code. Locally: `CheckpointCoordinatorTest` 17/17, `SharedCheckpointSchedulerTest` 10/10, `CheckpointManagerTest` 6/6, `spotless:apply` clean. This was raised by @SEZ9 on the STIP thread, and I agreed there to close it in this PR rather than separately, since the shared pool is what turns it from a per-pipeline stall into a member-wide one. ### Issue 1 — `BackpressureSlowSinkIT`, still open, and I could not reproduce it When I found the `thenAccept` path I thought it was a strong candidate cause for your Issue 1: under sustained backpressure the barrier round-trip is slow, so an inline body would occupy bounded dispatch threads for exactly as long as checkpoints are slow. That is the shape of the failure you described. I tried to confirm it and could not. Three local runs of `BackpressureSlowSinkIT#testCheckpointsKeepCompletingUnderSustainedBackpressure`, all green: | Run | Code | Conditions | Result | |---|---|---|---| | 1 | with the fix | `-XX:ActiveProcessorCount=2`, idle machine | pass, 125s | | 2 | **without** the fix (CI-equivalent) | `-XX:ActiveProcessorCount=2`, idle machine | pass, 125s | | 3 | without the fix | `-XX:ActiveProcessorCount=2`, 9 CPU hogs on 10 cores, load avg 10.7 | pass, 126s | Run 2 is the important one: it is the code CI failed on, under the pool bound CI would compute (`max(8, cores*2)` = 8 with `ActiveProcessorCount=2`), and it passed. So the inline continuation is **not** demonstrated to be the cause, and I am not claiming this commit fixes your Issue 1. Elapsed time was flat at ~125s across all three, so these runs were not near the failure margin at all. `ActiveProcessorCount` caps `availableProcessors()` but leaves real CPU throughput intact, and adding contention did not change the outcome either, so I read this as local conditions not reproducing a loaded shared runner rather than as evidence the test is fine. On your option (b): I found no issue or PR reporting `BackpressureSlowSinkIT` flakiness, and the test only merged on 2026-09-10 via #12107, so there is not much upstream history in either direction. What I think actually settles it, and what I would rather do than argue from a green local run: 1. Instrument the test's poll loop with dispatch-pool queue depth and wait time and let CI produce the numbers, since CI is the only environment reproducing this. This is your own suggestion and I think it is the right one. 2. Get the `dev`-head baseline for the same test in CI, so "already flaky" vs "caused by this PR" is answered with data rather than inference. Happy to do (1) in this PR behind the existing test, or as a separate instrumentation PR if you would rather keep this diff focused. CI is running on `31a454f35` now, which also gives a third data point on the same test for free. -- 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]
