Vivek1106-04 commented on PR #12418:
URL: https://github.com/apache/seatunnel/pull/12418#issuecomment-5776691815
@nzw921rx @DanielLeens On the question of which direction this harness
should take: I
would take the queuing-faithful standalone harness, and I can push it here.
Taking the two options as Daniel framed them.
**Driving the real
`CheckpointCoordinator#scheduleTriggerPendingCheckpoint`.** The hook
is there and is already `@VisibleForTesting protected`, so reaching it is
not the
problem. The problem is what it pulls in: `tryTriggerPendingCheckpoint` on
the happy path
calls `createPendingCheckpoint` and `startTriggerPendingCheckpoint`, so the
sample would
include checkpoint-id allocation, a real `PendingCheckpoint`, checkpoint
storage writes
and `CheckpointBarrierTriggerOperation` RPCs to task locations, on top of a
mocked
`CheckpointManager`, execution plan and task locations of the kind
`CheckpointCoordinatorTest`
builds with Mockito.
Two reasons I do not think that answers the question this benchmark was
built for.
First, the measurement stops being scheduling delay: barrier plus storage
dominates, and
the numbers would move with storage and RPC behaviour rather than with the
scheduling
model. Second, we already have that measurement -
`CheckpointingTimeBenchmark` is
barrier-ACK dominated, and its `CheckpointBenchmarkTrigger` reflects
straight into
`createPendingCheckpoint` and `startTriggerPendingCheckpoint`. Routing this
benchmark
through the same machinery would mostly reproduce it at P=1.
**The queuing-faithful harness.** @nzw921rx's objection is that the current
state arms
one trigger-like task per pool and so measures thread wake-up latency rather
than
queueing, and that is correct as written. Daniel's Issue 1 proposal fixes it
directly:
arm both a trigger-like task and a watchdog-like task per pool, with a duty
cycle long
enough that both of a pool's two workers can be occupied, so the measured
task actually
has to queue behind them. That is faithful to the engine's real pattern -
the trigger
re-arms itself at `scheduleTriggerPendingCheckpoint(checkpointType, 500L)`
while a
checkpoint is pending, and the timeout watchdog is armed per checkpoint, so
two timers
against a two-thread pool is what a busy pipeline actually looks like - and
it keeps the
measured quantity as scheduling delay.
It also fixes the reading of the current sweep, which is Daniel's other
point: the knee
at P=500 in what I posted is thread and CPU contention from roughly 1000
live threads in
one JVM, not queue pressure, and the harness as it stands cannot separate
those. With
both timers armed it can.
So unless you want the full-fidelity version for its own sake, I will:
1. add the watchdog-like task alongside the trigger-like one, with a
configurable duty
cycle, so a measured task can genuinely queue behind both workers of a
pool;
2. re-run the sweep and replace the numbers in the description, including
the numbers I
posted for #12165, since those were taken on the current harness and the
P=500 knee
in them should be read as thread contention rather than queueing;
3. say in the class Javadoc what the harness does and does not model, so the
next reader
does not have to rediscover it.
If you would rather have the real-engine-path version, or both arms, say so
and I will
do that instead - I would just rather not put a number in front of reviewers
without
being able to say which of the two it measures.
Separately, this PR's earlier three items remain addressed at this head: 4g
heap, README
reverted, and the class registered in the `benchmarks.yml` choice list. The
`benchmarks_core.txt` decision is still yours; my view is to leave it out
until the
matrix is settled, since eight parameter configurations at roughly four
minutes each is
about 32 minutes per Java version against the 240-minute nightly budget.
--
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]
