Vivek1106-04 commented on PR #12896: URL: https://github.com/apache/seatunnel/pull/12896#issuecomment-6316783458
Thanks @nzw921rx for the comparison run, and @DanielLeens for the review and the case for keeping it. I dropped `pipelineNum=1` in e9b40eca9. The sweep is now 10, 100 and 500 pipelines. At those counts the interval is at or above the 200 ms floor, so every point triggers 50 checkpoints a second. The added warmup goes from about 19 to about 9 minutes per JDK. A new test in `CheckpointSchedulingBenchmarkTest` fails if a pipeline count below the floor is added back. @DanielLeens, on your review notes: - **PR comparisons need a synced candidate:** I added this to the class Javadoc and the PR description. Scores from before this warmup are not comparable with scores after it. A candidate branched before this lands has to be rebased before it is compared. I will do that for #12165 when it is rerun. - **Flag test pins the literal:** I kept it as a config guard, as you suggested. The effect check is the per-iteration trend in the benchmark output. - **Empirical constants:** with the one-pipeline point gone, the 60 s minimum is what binds at every point (1,000 triggers take about 20 s at 50 a second). I kept the trigger count because it keeps the warmup long enough if a lower trigger rate is added later. I'm happy to drop it if you'd rather keep only the time. On the comparison: this PR is lower at every point, which is what more fully compiled code should give. P=500 shows a higher CV (2.2% to 3.2%) and error. With 2 runs per side I'm not reading much into it, but I'll look at the per-iteration data if a rerun shows it again. -- 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]
