DanielLeens commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-6359811546
Thanks @nzw921rx for dispatching the benchmark on top of #12896 and attaching the raw data, and thanks @Vivek1106-04 for syncing. I read the three comments against the unchanged head `771fce22127f51c0419f0633e8c95419a4b2493a` (static read only; I did not run or dispatch anything). This is a thread follow-up, not a new full review, because no commit has been pushed since the sync merge. My source verdict is unchanged. What the numbers say (median of 2 alternating runs on the same worker, `CheckpointScheduling.periodicTriggerDelay`, lower is better): | pipelineNum | Baseline (us/op) | Candidate (us/op) | Candidate CV | |---|---|---|---| | 10 | 91.967 | 135.108 | 12.42% (baseline 1.35%) | | 100 | 86.367 | 124.745 | 7.17% (baseline 2.95%) | | 500 | 106.186 | 122.286 | 3.59% (baseline 4.88%) | My reading (an inference from the posted table, not a measurement of mine): the candidate adds roughly 16 to 43 microseconds of trigger delay per due trigger, which is consistent with the extra timer-to-dispatcher hop this design introduces. That is tiny next to a checkpoint interval measured in seconds, and the PR's goal is the checkpoint-timer thread count, not trigger latency, so I do not read this as a functional regression. But it is a real, repeatable direction in the data, so it should be explained in the PR rather than left as a bare table. The 10-pipeline row also has a candidate CV of 12.42% with only 2 runs, so that row is not a stable estimate yet. The `RejectedExecutionException` in the second comment: the stack shows `checkpoint-dispatcher-N` running `PipelineCheckpointTask.run()` and then failing in `CheckpointCoordinator.createPendingCheckpoint` (`supplyAsync`) because the coordinator executor was already `Terminated`. That is a dispatch body racing the end-of-trial teardown, not a trigger that was lost mid-measurement (the same iteration reports 100 of 100 due triggers measured with 0 skipped). It is also exactly the behavior of my carried-over Issue 2: `PipelineCheckpointTask.java:100` rethrows after completing the future, so the error ends the dispatch worker and surfaces only through the default uncaught-exception handler on stderr. It stays Low severity, but this log is concrete evidence for it. Concrete next steps for @Vivek1106-04: 1. Post a short interpretation under the benchmark table: the per-trigger delay delta, why it comes from the extra hop, and that it is acceptable against checkpoint intervals. If you can, add a thread-count or memory data point for the same pipelineNum values, since that is the benefit this PR claims. 2. Ask for or run more than 2 rounds for the `pipelineNum=10` row, so the 12.42% CV can be judged. 3. Fix Issue 2: log the failing body with pipeline and job context inside `PipelineCheckpointTask.run()` and do not rethrow, since `completion` already carries the exception. That also removes the stderr noise in the benchmark output. If you prefer to keep it as is, please say so and I will keep it as a Low note. 4. Refresh the PR description (still says "elastic dispatch pool"; the code has a bounded pool). CI: the apache-side `Build` check is red; per my 10-22 note the earlier fork failure was `SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck`, which relates to the separate cancellation fix #12311 and not to this diff. I have not re-diagnosed the run for the new head; please post the fork run link for `771fce221` once it finishes. Status evidence: head `771fce22127f51c0419f0633e8c95419a4b2493a`, base `dev`, compare `diverged` (ahead 9, behind 12), mergeable state `blocked`. A committer's final approval is still needed; I cannot give it. -- 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]
