Vivek1106-04 commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-5760134825
@DanielLeens Benchmark results for this PR's design, and one finding that refines the residual cost we discussed on your Issue 2. @nzw921rx asked on #12339 for a dedicated benchmark so this change could be judged on measurements rather than argument. That benchmark is #12418. I ran it with a second arm for this PR's scheduler, both arms in the same JVM invocation, and the full write-up with every table is at https://github.com/apache/seatunnel/issues/12339#issuecomment-5760116122. Here is the part that bears on merging this PR. ## The design claim holds Scheduling delay, p50, us/op. `Mode.SampleTime`, 3 forks, JDK 17, `ActiveProcessorCount=4`, 1000 ms interval. | pipelines | per-pipeline pools (`dev`) | shared scheduler (this PR) | |---|---|---| | 1 | 7.12 | 10.74 | | 10 | 7.42 | 9.90 | | 100 | 9.62 | 9.95 | | 500 | 45.70 | 9.95 | Flat at about 10 us from 1 to 500 pipelines, against a knee between 100 and 500 for the current model. Live scheduler threads at P=500: 509 against at most 10 here. Crossover is at P=100; below that this PR is about 3 us slower, which is the extra timer-to-dispatcher hop, and which is three microseconds against a default interval of 300000 ms. ## Where "delay rather than drop" starts On Issue 2 you accepted the unbounded queue with the residual cost stated as: if every dispatch thread is blocked, further bodies queue instead of starting at once, reachable only when the member is already in trouble. That was the right call and I am not reopening it. The benchmark now puts a number on where it begins, at P=500 with 8 dispatch threads: | trigger body | dispatch utilization | p50 | |---|---|---| | 500 us | 3% | 9.90 | | 5000 us | 31% | 9.33 | | 20000 us | 125% | 460849 | Flat to 31%, then it collapses by about four orders of magnitude past saturation. And 461 ms is not a bound - the queue grows for as long as the overload lasts, the 10 second iteration just truncates the backlog. Worth saying plainly that I got this wrong on the first pass. Every configuration in my original sweep sat at 3% utilization, so body occupancy could not have mattered, and I read that as evidence it never matters. It was the wrong conclusion from a load that never tested it. ## What can occupy a dispatch thread I traced the body to see whether that cliff is reachable. It is `tryTriggerPendingCheckpoint`, and everything expensive is already delegated: the checkpoint id allocation goes through `supplyAsync(..., executorService)`, `triggerPendingCheckpoint` chains `thenApplyAsync`, `startTriggerPendingCheckpoint` is `thenAcceptAsync` since `31a454f35`, and `completePendingCheckpoint` takes the `this` monitor rather than `lock`, so its storage IO does not contend. What is left is atomics, timestamps, logging and a map put. Microseconds, and at the production default interval the utilization is effectively zero. One mechanism can hold a dispatch thread much longer. `startSavepoint` holds the coordinator lock across a sleep-poll: ```java synchronized (lock) { while (pendingCounter.get() > 0 && !shutdown) { Thread.sleep(500); } ``` `tryTriggerPendingCheckpoint` takes that same `lock` at line 791, so a periodic trigger firing during a savepoint window blocks on a dispatch thread until the in-flight checkpoint completes, which is seconds under backpressure. `CheckpointManager.triggerSavePoints()` fans out over every coordinator in a `parallelStream`, so one job suspend puts all of that job's pipelines into this state together. I want to be precise about the confidence here. The mechanism is verified in the code. Reaching eight simultaneous occupancies is **not** demonstrated - it needs periodic triggers to coincide with savepoint windows, a narrow coincidence at a 300 s interval, though the 500 ms re-arm on a pending checkpoint widens it. I have not reproduced it and I am not reporting it as a bug. The structural point is what I would ask you to weigh: this is a new cross-pipeline coupling. On `dev` a savepointing pipeline can block only its own two threads and nothing else is affected. With one shared bounded pool, enough concurrently blocked pipelines can delay unrelated pipelines' triggers. That is the same blast-radius argument this PR makes against the `thenAccept` path, pointed at the new design. It does not change my view that this PR is a net improvement - the numbers above are decisive at scale, and the steady-state body is microseconds. But it does mean the scheduling-delay metric should land ahead of any pool-size option rather than behind it, because it is exactly the detector for this: flat at 31% utilization, four orders of magnitude worse past 100%. Happy to chase the savepoint path properly if you want it settled before merge, or to file it as a follow-up. Your call on whether it blocks. Caveats: one machine, JDK 17, while the benchmark CI matrix is Java 8 and 11. #12418 registers the benchmark in `benchmarks.yml`, so it can be run on CI hardware against this branch if you want the figures confirmed there. -- 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]
