DanielLeens commented on PR #12418: URL: https://github.com/apache/seatunnel/pull/12418#issuecomment-5775116104
Thanks both for keeping this moving. @Vivek1106-04, the full sweep is useful and it actually lines up with Issue 1 from my earlier review: `triggerBodyMicros` (0 vs 500) barely moves the numbers at any pipeline count, which is exactly what you'd expect if the measured task almost never has to wait behind the periodic body. The knee at 500 pipelines looks like thread/CPU contention from ~1000 live threads in one JVM rather than scheduling-queue pressure, so I'd read this sweep as "per-pool wake-up latency under near-zero load," not yet as a load-bearing comparison for #12165 - which is really the same thing @nzw921rx is getting at below. @nzw921rx, I think you're right that the harness measures thread wake-up delay rather than queueing under the engine's real trigger pattern, and that this limits how far the numbers can be trusted as a stand-in for the actual scheduling model comparison - that's the same gap I flagged as Issue 1 (non-blocking) in my review. On routing through the real path: there's already a testing hook at that boundary - `CheckpointCoordinator#scheduleTriggerPendingCheckpoint(CheckpointType, long)` (`CheckpointCoordinator.java:547-553`) is `@VisibleForTesting protected`. The catch is that it schedules `tryTriggerPendingCheckpoint(checkpointType)`, which on the happy path calls `createPendingCheckpoint(...)` and `startTriggerPendingCheckpoint(...)` (`CheckpointCoordinator.java:816-824`) - i.e. it builds a real `PendingCheckpoint`, writes to checkpoint storage, and sends real RPC operations out to task locations. Driving the benchmark through that method means constructing a real (or fully mocked, as `CheckpointCoordinatorTest` does with Mockito) `CheckpointManager`, execution plan and task locations, so the sample would then include storage and RPC noise on top of scheduling delay, which is a different and heavier thing to measure than what this PR set out to isolate. A cheaper middle ground that gets at the same question without that machinery: my Issue 1 write-up already proposed arming both a trigger-like task and a watchdog-like task per pool with a duty cycle long enough that both of a pool's 2 workers can be occupied, so the measured task genuinely has to queue behind them - closer to how `tryTriggerPendingCheckpoint`'s own re-scheduling (`scheduleTriggerPendingCheckpoint(checkpointType, 500L)` when `pendingCounter.get() > 0`, line ~816) can pile a second timer onto the same pool. That would show whether pool width matters under real contention, still as a standalone harness. Could you say which of these you're looking for: exercising `tryTriggerPendingCheckpoint`'s full side effects (storage + RPC, higher fidelity but a much bigger, more intrusive change), or a queuing-faithful standalone harness along those lines? Either is a reasonable ask, but they're different asks with different cost, and it'd help @Vivek1106-04 to know which one unblocks this PR versus which is really scoped to the #12165 discussion. -- 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]
