Vivek1106-04 commented on PR #12418:
URL: https://github.com/apache/seatunnel/pull/12418#issuecomment-5776906770
@nzw921rx Our comments crossed - mine went up about three minutes after
yours and was
written without having read this. Please take this one as my actual answer,
because your
design changes my view.
**You are right and my objection does not apply to your version.** I argued
against
routing through the real path on the grounds that the sample would be
dominated by
storage and RPC. Your measurement boundary removes that: it starts at
`scheduleTriggerPendingCheckpoint` and ends when `executorService` receives
the
checkpoint-creation task, so everything I was worried about - id allocation,
`PendingCheckpoint` construction, barrier RPC, ACK, storage, IMap - is
outside the
measured region by construction. That is a better answer than the standalone
harness I
was proposing, because it measures the real `tryTriggerPendingCheckpoint`
including its
state checks and its `synchronized (lock)`, which is exactly the part the
two scheduling
models differ in. I am happy to build it.
Five things I checked against the code before replying. One of them I think
removes work
from your proposal rather than adding it.
### 1. The testing constructor is not needed, and cannot work as written
`dev` and #12165 already share the identical 11-argument public constructor.
The
difference is entirely inside it:
- `dev` builds its own pool there: `this.scheduler =
Executors.newScheduledThreadPool(...)`
(`CheckpointCoordinator.java:239-250` on `dev`)
- #12165 leases instead: `this.scheduler =
manager.leaseCheckpointScheduler(pipelineId)`
So a benchmark that constructs a coordinator through the existing public
constructor gets
each revision's real scheduler automatically, with one source file that
compiles on both.
A constructor taking `PipelineCheckpointScheduler` could not compile on
`dev` at all -
that type does not exist there - so it would force two divergent benchmark
sources, which
is the thing the A1/B1/B2/A2 comparison most needs to avoid.
It also keeps this PR test-only. Adding a constructor to
`CheckpointCoordinator` makes a
benchmark PR touch production engine code, which I would rather not do
without a
committer explicitly asking for it.
### 2. The interval guard will stop the probe firing after the first sample
This is the one that needs a decision from you, because it shapes the
fixture.
`tryTriggerPendingCheckpoint` returns early, after re-arming itself, when
the trigger is
inside the configured interval (`CheckpointCoordinator.java:782-799`):
```java
long interval = currentTimestamp - latestTriggerTimestamp.get();
if (interval < coordinatorConfig.getCheckpointInterval()) {
scheduleTriggerPendingCheckpoint(
checkpointType, coordinatorConfig.getCheckpointInterval() -
interval);
return;
}
```
`latestTriggerTimestamp` is only set in `createPendingCheckpoint`
(`CheckpointCoordinator.java:1126`), so after the first successful trigger
every
subsequent `scheduleForBenchmark()` inside the interval takes that branch
and never
reaches `executorService`. The armed latch would then wait for the re-armed
trigger
instead of the one the sample intended. There is a second guard right after
it for
`checkpoint.min-pause`.
Three ways out, and I would like your call:
- pin `checkpointInterval` to the 10 ms minimum and `checkpointMinPause` to
-1, which
bounds sampling at roughly 100/s but keeps the production code path
completely intact;
- reset `latestTriggerTimestamp` between samples, which means reaching into
coordinator
state - the kind of override you asked to avoid;
- treat the guarded path as the measured one deliberately, since the 500 ms
re-arm at
`CheckpointCoordinator.java:841` is itself a real production behaviour.
I lean to the first: slow but honest, and JMH `SampleTime` copes with a low
rate.
### 3. The single-shot probe can be tripped by the previous sample
`arm()` / `getAndSet(null)` fires on the next `execute()`, but the
coordinator posts a lot
to that same executor - `whenCompleteAsync`, `thenApplyAsync`,
`thenAcceptAsync` in
`startTriggerPendingCheckpoint`, and as of `da0cbbf52` on #12165 the
checkpoint-expiry
hop as well. If the previous sample's completion work is still draining, the
probe closes
on that instead of on the trigger. `awaitCheckpointCompleted()` helps, but I
would rather
the probe identify the submission than trust quiescence - simplest is for
the fixture to
recognise the task it is waiting for, and to fail the sample loudly if it
sees an
unexpected submission rather than silently recording the wrong number.
### 4. Mockito is not available in `seatunnel-benchmarks`, and these are
main sources
The benchmarks live in `src/main/java`, so anything they need becomes a
compile-scope
dependency of a shipped module. Mockito is not currently a dependency there.
For the IMap
this is easy to avoid: a `java.lang.reflect.Proxy` over a
`ConcurrentHashMap` covers
`get` / `set` / `compute` / `remove` in about twenty JDK-only lines.
`CheckpointManager`
is a class rather than an interface, so it cannot be proxied - that one
needs either
Mockito added to this module, or a real manager.
### 5. There is already a no-mock route to a real coordinator
`SeaTunnelCheckpointEnvironmentContext` starts in-JVM master and worker
instances for
`CheckpointingTimeBenchmark`, so a real `CheckpointManager`, real IMap and
loopback RPC
are reachable today without mocking anything. The cost is a heavier fixture
per fork, and
the probe then needs a different seam, since the executor belongs to
`CoordinatorService`
rather than to us.
### What I would like from you
Point 1 removes the constructor change, and points 2 to 4 are fixture
details I can
settle myself once you have answered point 2. The one genuine fork is
whether you want
the no-cluster fixture you sketched, with a proxied IMap and Mockito added
here for
`CheckpointManager`, or the mini-cluster route in point 5 that needs no
fakes at all.
Say which, plus your preference on the interval guard, and I will push it.
If you would
rather see it than discuss it, I can push the no-cluster version with the 10
ms interval
and the existing constructor, and we can argue with numbers in front of us.
--
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]
