Thank you @Vivek1106-04 for the careful write-up and for tackling the warmup drift in CheckpointSchedulingBenchmark. I reviewed the whole diff at head 1914eabf66a2ac296140341581f8c4ca049eae9d and traced how the benchmark is launched, so this is a full review rather than a skim.

What Problem Does This PR Solve?

• User pain point: CheckpointSchedulingBenchmark scores keep falling across measured iterations, because the checkpoint trigger path runs only 5 to 50 times a second and the JIT compiles by invocation count. The measurement therefore records the warmup curve, and the one-pipeline point (200 ms interval floor, 5 triggers per second) warms up ten times slower than the other points.
• Fix approach: Run the forks with -XX:CompileThresholdScaling=0.05, and let each trial's own coordinators trigger 1,000 checkpoints (and run for at least 60 s) before JMH's warmup starts; the trigger count comes from a counter in the fake checkpoint manager. Javadoc documents the one-pipeline point as an idle-member point.
• One-sentence summary: A benchmark-only change that makes the scheduling benchmark measure steady-state, compiled code instead of the JIT warmup, with no production code touched.

1. Code Change Review

1.1 Core Logic Analysis

All 5 changed files are inside seatunnel-benchmarks; nothing under seatunnel-engine, seatunnel-api or seatunnel-core is modified, so there is no production-path impact.

// CheckpointSchedulingBenchmark.setUp (Level.Trial)
// before: new CheckpointSchedulingFixture(pipelineNum, interval)
// after:  new CheckpointSchedulingFixture(pipelineNum, interval,
//             TRIGGER_PATH_WARMUP_TRIGGERS /*1_000*/, 60s in millis)   // CheckpointSchedulingBenchmark.java:158-163

// CheckpointSchedulingFixture.setUp()
// startStaggered(); awaitWarmupTriggers(); resync(1);                  // CheckpointSchedulingFixture.java:197-200

Key findings:

• awaitWarmupTriggers() (CheckpointSchedulingFixture.java:213-233) waits until the shared AtomicLong has advanced by warmupTriggers and the minimum time has passed. The counter is incremented once per CheckpointBarrierTriggerOperation sent by the fake manager (FakeTaskCheckpointManager.java:131), i.e. exactly the trigger path that is measured. The loop exits on both conditions, so a slow start cannot skip the minimum time.
• The failure deadline is sound at every configured point. I computed max(minWarmup, 2 * triggers * interval / pipelines) + 3 intervals + 1 s: 1 pipeline, about 401 s against about 200 s needed; 10 pipelines, 61 s; 100 pipelines, 67 s; 500 pipelines, about 91 s against about 60 s needed. There is at least a 1.5x margin everywhere, and a stall raises IllegalStateException with a useful message instead of hanging.
• resync(1) after the warmup (CheckpointSchedulingFixture.java:200) still waits for every coordinator to have triggered, so the thread-count report and the probe phases are taken on the warmed state, as the Javadoc claims.
• The new 4-argument constructor is additive, and the old 2-argument constructor delegates with 0, 0, so existing callers (the other fixture tests) keep their previous behavior.
• -XX:CompileThresholdScaling=0.05 sits in @Fork(jvmArgsAppend) (CheckpointSchedulingBenchmark.java:98). tools/benchmarks/run_benchmarks.sh overrides only the fork count, warmup and measurement iterations on the command line, not jvmArgsAppend, so the flag reaches the forked JVMs in CI.

Runtime path:

workflow_dispatch(CheckpointSchedulingBenchmark) -> run_benchmarks.sh -> JMH fork (flag applied)
  -> @Setup(Trial): fixture.setUp()
       -> startStaggered()  (coordinators armed across one interval)
       -> awaitWarmupTriggers()  (>= 1000 triggers and >= 60 s, counted in FakeTaskCheckpointManager)
       -> resync(1)
  -> JMH warmup + measurement iterations on compiled code

The normal benchmark run reaches the changed code on every trial setup. Note that CheckpointSchedulingBenchmark is not part of the nightly benchmarks_core suite (tools/benchmarks/suites/benchmarks_core.txt lists only the pipeline and checkpointing-time benchmarks), so the cost only applies to manual dispatch of this class, a custom selector, or a full .* run.

1.2 Compatibility Impact

Fully compatible for users and for the product: no config, API, wire or state format is touched. For benchmark consumers there is one expected change: scores from CheckpointSchedulingBenchmark before and after this PR are not comparable, because both the JIT flag and the warmup protocol change. That is inherent to fixing the measurement, but see item 2 below for the PR-comparison workflow.

1.3 Performance / Side-Effect Analysis

• The fixture adds one AtomicLong.incrementAndGet() per trigger on the measured path (FakeTaskCheckpointManager.java:131). At 5 to 50 triggers per second per member this is far below the microsecond-scale scores and is the same for every revision, so I consider it negligible.
• Added wall time: 1,000 triggers at 1 pipeline is about 200 s, and at least 60 s for each of the other three points, so about 380 s per trial, roughly 19 minutes per JDK for the 3-fork sweep, as the description says. In the PR-comparison mode of run_benchmarks.sh (four outer runs of one fork each) this is about 25 minutes per JDK. Both stay far below the 240-minute job limit when only this class is selected.
• CompileThresholdScaling=0.05 changes the JIT behavior of the code being measured: it compiles with much less profile data than a production JVM. The scores are therefore good for comparing revisions, but they are not an absolute statement about production latency.

1.4 Error Handling and Logging

The numbered items are all non-blocking.

1. Low: the flag test pins the string, not the effect. Location: CheckpointSchedulingBenchmarkTest.java:47-54. Problem: shouldLowerJitThresholdsSoTheTriggerPathCompilesWithinWarmup asserts that the annotation contains -XX:CompileThresholdScaling=0.05, which would go red if the flag is removed, but it duplicates the literal and cannot prove the path actually compiles in time. Potential risk: a future edit that changes the literal in both places keeps the test green while changing the measurement. Best improvement: keep the test as a config guard (it is useful for that) and rely on the JMH output iterationReport() as the effect check, or share one constant between the annotation and the test. Raised by another reviewer: no.
2. Low: PR comparison depends on both sides carrying the same warmup protocol. Location: tools/benchmarks/run_benchmarks.sh (ABBA sequence) together with CheckpointSchedulingBenchmark.java:98. Problem: in PR mode, the baseline jar is built from seatunnel_ref (default dev) and the candidate jar from the PR head, each with its own benchmark classes. After this PR lands, a candidate branched before it will be measured without the flag and the new warmup against a baseline with them. Potential risk: a misleading regression/improvement report for CheckpointSchedulingBenchmark on a stale branch (for example the PR #12165 that motivated this). Best improvement: add a sentence to the Javadoc or the PR description that candidates must be synced with dev before using this benchmark for comparison. Raised by another reviewer: no.
3. Low: duplicated timing constants without a shared definition. Location: CheckpointSchedulingBenchmark.java:116-123 and the Fork annotation at :98. Problem: the reasoning for 1_000 triggers and 60 seconds lives in Javadoc only; they cannot be derived from the code. Best improvement: none required; the Javadoc is already clear. Mentioned only so the numbers get revisited if the trigger path changes tier behavior. Raised by another reviewer: no.

On the two open choices in the description (I am happy with either, so this is a suggestion):

• Choice 1 (CompileThresholdScaling vs defaults): I would keep the flag. The measurement goal is comparing revisions, and a 150 s default-threshold warmup would still leave the one-pipeline body at a lower tier, as the PR notes. The trade-off to document is item 1.3's last bullet.
• Choice 2 (cost / dropping the one-pipeline point): I would keep the one-pipeline point, since it is documented as the idle-member point and the extra time only applies to manual runs of this class (it is not in benchmarks_core).

2. Code Quality Assessment

2.1 Coding Standards

New files are not added, imports are explicit, and the Javadoc explains why (JIT by invocation count, trigger-count warmup, and the deadline formula). Spotless-style formatting looks consistent.

2.2 Test Coverage and Test Stability

• shouldTriggerTheWarmupCheckpointsBeforeMeasuringAnything asserts getTriggeredCheckpoints() >= 40 right after setUp() and then samples 20 triggers. If awaitWarmupTriggers() were removed, setUp() would return after roughly one trigger per coordinator (4 total), so the assertion would fail. This is a real regression guard.
• shouldKeepWarmingUpForTheMinimumTimeOnceTheTriggersRan uses 1 trigger and a 5 s minimum and asserts the elapsed time of setUp() is at least 5 s. Dropping the minimum-time condition makes it fail, because the rest of setup takes about a second.
• Timing margin: the first test has about 2 s of expected work against a computed 5.6 s deadline (4 pipelines, 200 ms interval, 40 triggers); the second exits at 5 s against a 6.6 s deadline. This is sufficient for normal runners, though a heavily loaded Windows runner is the place to watch for flakiness. Because the exit condition for the minimum time is a lower bound, it cannot flake by running too fast.
• No existing assertion was weakened; the PR only adds tests.
• CI for the head SHA: the Build check is green (success).
• Stability rating: Risk present (low), only because both new fixture tests use real coordinators and wall-clock waits of several seconds.

2.3 Documentation Updates

No user documentation covers this benchmark and the change is benchmark-only (no docs/en / docs/zh page applies). The class Javadoc was updated and is accurate against the code: 200 s at one pipeline, 60 s elsewhere.

3. Architectural Soundness

3.1 Elegance

Precise fix. Counting triggers rather than seconds is the right unit because the JIT compiles by invocation count, and using the real coordinators as the warmup subject avoids the deoptimization problem the author found with throwaway coordinators.

3.2 Maintainability

Good: the new behavior is behind a constructor overload with a documented zero value, and the counter is a single AtomicLong shared by the manager and the fixture.

3.3 Extensibility

Other fixture-based benchmarks can reuse the same "warm up in triggers" idea by passing a different count. The 1,000 figure is an empirical GitHub-runner value, so it may need a revisit when the trigger path changes.

3.4 Historical-Version Compatibility

No effect on product compatibility. Historical benchmark scores for this class are not comparable (see 1.2).

4. Issue Summary

5. Merge Recommendation

CI state of head 1914eabf6: Build completed with success. The PR is 73 commits behind dev but reports as mergeable with no conflicts; it is blocked only by the review requirement.

Conclusion: Ready to merge

1. Blockers (must fix)
   • None.
2. Recommended fixes (non-blocking)
   • Add a short note that PR comparisons need the candidate synced with dev (item 2), and optionally share the flag literal with the test (item 1).

Overall assessment: The change is small, confined to the benchmark module, and the warmup logic and its failure deadline are correct for every configured pipeline count. A committer approval is still needed, as I cannot approve on their behalf. I did not run the code; this review is based on the full diff, the launch scripts and the CI result.

Better alternative: None needed. If the empirical numbers drift, a later change could make the warmup adaptive by waiting until the trigger-path latency stops improving instead of using a fixed count.

Thank you again for the thorough analysis behind this fix.

Thank you for your prompt follow-up. I believe our benchmark testing goal is to be more coordinated and efficient, so pipeLine=1 seems like we can remove it?

Thank you @Vivek1106-04 for acting on the earlier feedback so quickly, for dropping the one-pipeline point, and for @nzw921rx's comparison run. I re-read the whole diff at head e9b40eca9d0e5b3bcc61b3b9e5d5f36338e18a2e from scratch (not only the new commit), so this is a full re-review.

Prior findings from my review of 1914eabf6:

• Item 1 (flag test pins the literal): kept as a config guard, as I suggested. Acknowledged, no change needed.
• Item 2 (PR comparison needs a synced candidate): resolved. The class Javadoc now says scores before and after this warmup are not comparable and the candidate must contain it.
• Item 3 (empirical constants only in Javadoc): acknowledged, Javadoc is clear. Resolved.
• Open choice about the one-pipeline point: I had said I was fine either way; I am happy with the decision to drop it, and I withdraw my suggestion to keep it. The interval floor really makes that point a different workload (5 triggers a second vs 50), and the new test guards it.

What Problem Does This PR Solve?

• User pain point: CheckpointSchedulingBenchmark scores keep falling across measured iterations, because the trigger path runs only 5 to 50 times a second and the JIT compiles by invocation count, so the measurement records the warmup curve.
• Fix approach: Fork JVMs run with -XX:CompileThresholdScaling=0.05; each trial lets its own coordinators trigger 1,000 checkpoints and run for at least 60 s before JMH warmup; the sweep now starts at 10 pipelines so the 200 ms interval floor never slows the trigger rate.
• One-sentence summary: A benchmark-only change that makes the scheduling benchmark measure compiled steady-state code instead of the JIT warmup, with no production code touched.

1. Code Change Review

1.1 Core Logic Analysis

All 5 changed files are in seatunnel-benchmarks; nothing under seatunnel-engine, seatunnel-api or seatunnel-core changes, so there is no production-path impact.

// CheckpointSchedulingBenchmark.CoordinatorsState
// before: @Param({"1", "10", "100", "500"})
// after:  @Param({"10", "100", "500"})
// setUp(): new CheckpointSchedulingFixture(pipelineNum, max(200ms, pipelineNum * spacing),
//             TRIGGER_PATH_WARMUP_TRIGGERS /*1_000*/, 60 s in millis)

// CheckpointSchedulingFixture.setUp()
// startStaggered(); awaitWarmupTriggers(); resync(1);

Key findings:

• awaitWarmupTriggers() (CheckpointSchedulingFixture.java, after setUp()) takes the counter target after startStaggered(), then loops until both the trigger count and the minimum time are reached. The counter is incremented in FakeTaskCheckpointManager.sendOperationToMemberNode once per CheckpointBarrierTriggerOperation, which is exactly the measured trigger path.
• The failure deadline is max(minWarmup, 2 * triggers * interval / pipelines) + 3 intervals + 1 s. With the new sweep: 10 pipelines (200 ms interval) gives 60 s floor and 40 s of work, 100 pipelines (2 s interval) also 40 s of work, 500 pipelines (10 s interval) 40 s of work. In every case the 60 s minimum binds and the real trigger time is about 20 s, so there is a 3x or larger margin before the deadline fires, and a stall produces an IllegalStateException with a clear message instead of a hang.
• Removing 1 from the sweep removes the only point where the deadline was tight (about 200 s needed against about 401 s allowed), so the previous margin concern is gone.
• The 4-argument constructor is additive; the 2-argument constructor delegates with 0, 0, so other callers keep the old behavior.
• -XX:CompileThresholdScaling=0.05 is in @Fork(jvmArgsAppend); tools/benchmarks/run_benchmarks.sh overrides only fork/iteration counts, so the flag reaches the forked JVMs.

Runtime path:

workflow_dispatch(CheckpointSchedulingBenchmark) -> run_benchmarks.sh -> JMH fork (flag applied)
  -> @Setup(Trial): fixture.setUp()
       -> startStaggered()
       -> awaitWarmupTriggers()  (>= 1000 triggers and >= 60 s, counted in FakeTaskCheckpointManager)
       -> resync(1)
  -> JMH warmup + measurement on compiled code

The normal benchmark run reaches the changed code on every trial setup. The class is still not in benchmarks_core.txt, so the cost applies only when it is selected on purpose.

1.2 Compatibility Impact

Fully compatible. No config, API, wire or state format is touched. Benchmark consumers should note that scores for this class before and after this PR are not comparable, which the Javadoc now states.

1.3 Performance / Side-Effect Analysis

• One AtomicLong.incrementAndGet() per trigger on the measured path is negligible at 50 triggers a second and identical for every revision.
• Added wall time is now about 9 minutes per JDK (3 points x 3 forks x 60 s), down from about 19. Well under the job limit.
• CompileThresholdScaling=0.05 compiles with less profile data than a production JVM, so scores are for comparing revisions, not absolute production latency. This is stated in the PR description.
• The sweep loses the one-pipeline "idle member" baseline. This is an accepted trade-off, agreed in review.

1.4 Error Handling and Logging

Issue 1

• Location: CheckpointSchedulingBenchmarkTest.java, shouldOnlySweepPipelineCountsTheIntervalFloorDoesNotSlowDown
• Problem description: the guard reads only paramValues("triggerSpacingMillis")[0]. If a second spacing value is added to @Param({"20"}) later, a pipeline count that is fine at the first spacing but below the 200 ms floor at the other would not be caught.
• Potential risk: the guard silently weakens when the sweep gains a second spacing.
• Best improvement: loop over all spacing values (a nested loop, or test against the smallest one).
• Severity: Low
• Raised by another reviewer: No

No other error-handling concerns: the failure path keeps checkFailure() in the wait loop so a failed coordinator surfaces immediately.

2. Code Quality Assessment

2.1 Coding Standards

The new constants, the 4-argument constructor and awaitWarmupTriggers() all carry comments that explain why (JIT by invocation count, deadline formula, minimum time). No new files, imports are explicit, formatting looks Spotless-consistent. The getTriggeredCheckpoints() package-private accessor is test-only and self-explanatory.

2.2 Test Coverage and Test Stability

• shouldTriggerTheWarmupCheckpointsBeforeMeasuringAnything asserts at least 40 triggers right after setUp(); without awaitWarmupTriggers() setup returns after roughly one trigger per coordinator, so it would fail.
• shouldKeepWarmingUpForTheMinimumTimeOnceTheTriggersRan asserts setup takes at least 5 s with a 1-trigger target; dropping the minimum-time condition makes it fail.
• shouldOnlySweepPipelineCountsTheIntervalFloorDoesNotSlowDown fails if pipelineNum=1 is added back (1 x 20 ms is below 200 ms).
• shouldLowerJitThresholdsSoTheTriggerPathCompilesWithinWarmup is a config guard on the annotation literal, as discussed.
• No existing assertion was weakened; the PR only adds tests.
• Timing: the two fixture tests wait about 2 s and 5 s in real time against deadlines of about 5.6 s and 6.6 s; the minimum-time condition is a lower bound, so it cannot flake by running fast. A heavily loaded Windows runner is the place to watch.
• Stability rating: Risk present (low), only because of the wall-clock waits.

CI for head e9b40eca9: Build is green (success, 59m48s), labeler and Notify test workflow are green. The PR is about 81 commits behind dev and reports MERGEABLE with no conflicts; it is only blocked on the required review.

2.3 Documentation Updates

No docs/en / docs/zh page covers this benchmark and there is no user-facing change. The class Javadoc is updated and matches the code (10-pipeline start, 50 triggers a second, 60 s minimum).

3. Architectural Soundness

3.1 Elegance

Precise fix. Counting triggers matches how the JIT decides to compile, using the real coordinators avoids the deoptimization problem of throwaway ones, and starting the sweep above the interval floor removes the root cause of the slow one-pipeline point instead of working around it.

3.2 Maintainability

Good. The behavior sits behind an additive constructor with documented zero values, and a single AtomicLong is shared by manager and fixture.

3.3 Extensibility

The trigger-count warmup is reusable by other fixture-based benchmarks. The 1,000 and 60 s values are empirical for GitHub runners and should be revisited if the trigger path changes.

3.4 Historical-Version Compatibility

No product impact. Historical scores for this class are not comparable, which is documented.

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers - must be fixed
   • None.
2. Recommended fixes - non-blocking
   • Optionally make the floor guard iterate all spacing values (Issue 1).

Overall assessment: All three of my earlier notes are resolved or acknowledged, the one-pipeline point is dropped as discussed, and the warmup logic and its failure deadline are correct for every configured point. On the comparison run posted by @nzw921rx: the score deltas are consistent with more fully compiled code, and the P=500 CV change from two runs per side is too small a sample to act on, as the author says. I did not run the code; this review is based on the full diff, the launch scripts and the CI result. A committer approval is still required to merge.

Better alternative: None needed. If the empirical numbers drift later, an adaptive warmup that waits for trigger latency to stabilize could replace the fixed count.

Approving on head e9b40ec. Thanks @Vivek1106-04 for the careful follow-up: the Javadoc, flag-literal guard and documented constants from the earlier review are all in place, and the change is benchmark-only with no engine, API or core impact. Build, labeler and Notify checks are green. The one remaining Low nit (the floor-guard test reads only the first spacing value) is optional and can be a follow-up. Great work!

+1 LGTM