DanielLeens commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-5683816451
Replying to your latest round of comments (review comment, and three issue-level comments from today) together, since they're all design/process-level rather than line-specific. ### On "excessive solutions" / marking this experimental (review comment) > Have you considered excessive solutions on your end? For example, clearly defining it as experimental ability? I don't think an experimental flag fits this particular change, for a structural reason rather than a dismissal of the concern: this PR has no new `Option`, no config default change, and no checkpoint/savepoint format or protocol change — I verified that directly in my 2026-09-10 review (no new config surface, `git grep` across `docs/en`/`docs/zh` found nothing referencing the old per-pipeline thread-naming pattern to update). The old per-coordinator pool code path is fully removed, not kept behind a switch, so there's no clean on/off seam to gate as "experimental" — it's either correct for everyone or it isn't. If the community wants a staged rollout for a change like this, I'd suggest that's better done via the normal RC/soak cycle than an in-product experimental toggle, since the architecture doesn't leave a natural place to put one. That said, I do share the underlying worry about validating this carefully before it reaches everyone, which is exactly why my CHANGES_REQUESTED review is still blocking on a clean CI run rather than approving from code-reading alone (see CI note below). ### On scheduling efficiency / large-scale latency (review comment) and head-of-line blocking (issue comment) > I am concerned about the scheduling efficiency and latency of large-scale tasks > If thread scheduling slows down due to IO or a certain jobId, the CK of subsequent tasks will be delayed. Do we have any better solutions? This is exactly the tradeoff the timer/dispatch split is built to avoid for the "arm the next trigger" step. The shared `checkpoint-timer` pool only ever runs `() -> submit(task)` (`SharedCheckpointScheduler.java:107`) — a single non-blocking `dispatcher.execute(task)` hand-off. It never runs the actual checkpoint body, so a slow or blocked pipeline cannot stall another pipeline's timer from firing on schedule. The real work (the checkpoint body, which can call into storage/RPC) runs on `dispatcher`, and `dispatcher` is unbounded (`newCachedThreadPool`), so job A's slow IO occupies its own dedicated thread there rather than contending for a shared, limited worker with job B's checkpoint body. That said, it is still a shared JVM-wide resource in a broader sense: if enough pipelines are simultaneously blocked on slow IO at very large scale, you could see member-wide resource pressure (thread count, context-switching, memory) even though no single mutex or bounded queue is shared between jobs. That's the same tradeoff already tracked as Issue #2 (Low, non-blocking) in my CHANGES_REQUESTED review — bounding and making `dispatcher`'s pool size configurable is the natural follow-up. So: for the direct "job A blocks job B's trigger" scenario, the design already avoids it structurally today; for member-wide pressure at extreme scale, that's the same known, already-flagged carryover rather than a new gap. ### On CI / rebase (issue comment) > CI error, please rebase upstream/dev Agreed, and this matches the actual blocker in my 2026-09-10 CHANGES_REQUESTED review: CI was red on the head commit for reasons I traced back to being unrelated to this PR's own diff, and the branch was already well behind `dev` at that point. The head commit is still the same one I reviewed then, so this is still open — rebasing onto current `dev` and getting one clean CI run is the right next step before this can move forward. ### On STIP (issue comment) > I think it should comply with the STIP principle... You can refer to the following link to initiate STIP Reasonable suggestion given the scope — this does touch core, node-wide checkpoint-scheduling internals, so going through STIP for broader design visibility and consensus makes sense to me. That said, whether to require it is a process call for the author and the PMC/maintainers to make, not something I can gate as a reviewer. -- 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]
