Vivek1106-04 commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-5691753289
@nzw921rx Thanks for the review. Answering your three points. ### 1. Rebased Rebased onto current `dev` and force-pushed. Head is now `7310dd7a2`. Locally: `spotless:check` clean, full `-am` build of `seatunnel-engine-server` and `seatunnel-e2e-common` succeeds, and `CheckpointCoordinatorTest` (16) plus `SharedCheckpointSchedulerTest` (8) pass. CI is running on the new head. ### 2. "If thread scheduling slows down due to IO or a certain jobId, the CK of subsequent tasks will be delayed" This is the right thing to check, and it is the reason the scheduler is split into two pools rather than one. A timer thread never runs checkpoint work. In `SharedCheckpointScheduler`, `dispatchAfter` schedules `() -> submit(task)` on the timer, and `submit` does nothing but `dispatcher.execute(task)`. The dispatcher is a cached pool, so `execute` neither blocks nor queues: it hands the task to an idle thread or creates one. The timer thread's work per firing is a constant-time, non-blocking hand-off. So a checkpoint that blocks on IO or a slow RPC occupies a dispatcher thread, not a timer thread. The next periodic trigger for every other pipeline still fires on schedule, and so does the next one for the slow pipeline itself. Only two things are ever scheduled, both in `CheckpointCoordinator`: - the periodic re-arm, `scheduleTriggerPendingCheckpoint` -> `tryTriggerPendingCheckpoint` - the checkpoint-timeout watchdog, armed inside `startTriggerPendingCheckpoint` Both of those bodies run on the dispatcher. Worth noting this is stricter than what `dev` does today. On `dev` each coordinator builds `Executors.newScheduledThreadPool(2)` and runs the bodies on those same two threads. A checkpoint body that blocks there consumes one of that pipeline's own two timer threads, and a blocked trigger plus a blocked watchdog can occupy both, delaying that pipeline's own next tick. After this PR that self-interference is gone as well, because the timing threads are no longer the ones doing the work. The honest caveat: the dispatch pool is unbounded. If N pipelines all block at once, up to N dispatcher threads exist for as long as they block, and idle ones are reaped after the cached pool's 60s keep-alive. That worst case is no worse than `dev`'s fixed two-threads-per-pipeline, and the steady state improves from `2 * pipelines` threads to 2 timer threads per member plus whatever is actively running. I deliberately did not bound the dispatch pool. A bounded pool over a `SynchronousQueue` rejects when saturated, and a rejected task here is a silently dropped checkpoint trigger or a silently dropped timeout watchdog, which is worse than an extra thread. Bounding it properly needs a rejection policy, and that is a design question rather than something to decide inside this PR. ### 3. Experimental flag @DanielLeens already covered this in https://github.com/apache/seatunnel/pull/12165#issuecomment-5683816451, and I agree with their reading, so this is just me confirming it as the author rather than a new argument. I do not think a flag fits, for a structural reason. The PR adds no new `Option`, changes no default, and changes no checkpoint or savepoint format or protocol. The behavior change is entirely "which thread runs the timer". There is no user-visible surface for an experimental flag to gate, so the only way to make it switchable would be to keep both the old per-coordinator pool and the new shared one alive behind a branch, which doubles the code under test for no configuration benefit. If the concern is that the scheduling change should be reversible in the field, the cleaner form of that is the STIP discussion below rather than a config toggle. ### 4. STIP Agreed. This touches node-wide checkpoint scheduling internals in the engine, which fits the "engine-level change" bar. I will open `[STIP-40][Improve][Zeta]` covering the design, the alternatives, the unbounded-dispatch-pool tradeoff above, and the test plan, and post to `[email protected]` first. This PR will reference the STIP number once it is open. -- 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]
