DanielLeens commented on PR #11986: URL: https://github.com/apache/seatunnel/pull/11986#issuecomment-5459120647
Thanks @SEZ9 for the deep second pass — I read the head at `83b7f5d5631b` again myself rather than taking your findings at face value, since I'd already called this "ready to merge" and want to be honest about what actually changes that. Per-issue verdicts below, each backed by source I re-checked directly: **Issue 7 (`@Param` fields declared `private`) — confirmed, and I'd treat this as effectively blocking, not Medium.** I pulled the current head file directly: `BlockingQueueState.capacity`/`recordPoolSize` and `DisruptorQueueState.capacity`/`recordPoolSize` in `IntermediateQueueBenchmark.java` are all `private`. JMH's annotation processor generates a separate class (in the same package, but still a distinct class) that has to write these fields from outside the declaring class — plain Java access rules mean `private` blocks that regardless of package, so this should generate a processor error, not just a style nit. This also explains why "CI is green" doesn't clear it: as I noted in my own last review, `seatunnel-benchmarks` only builds under the non-default `benchmark` Maven profile (`pom.xml:1200-1208`, `activeByDefault=false`), so the default `Build` check never compiles this module or runs the JMH generator at all — this bug is currently invisible to the check that's sho wing green. I haven't actually run a local `-Pbenchmark` build to execute-verify the generator failure (staying within the no-local-build policy for this repo), so I'm reasoning from JMH's well-documented private-field limitation rather than a build log — @nzw921rx, can you confirm with a local `-Pbenchmark` build or point at a benchmarks `workflow_dispatch` run that actually exercised this class? If it does fail to generate, this needs a fix before merge regardless of what my last review said. **Issues 1/3/6 (Disruptor consumer exceptions never reach `consumerFailure`) — confirmed accurate.** Traced `IntermediateQueueBenchmarkState.setUp()` (line 80) → `IntermediateDisruptor.collect()` → `RecordEventHandler.onEvent()`/`handleRecord()`: `collector.collect(record)` is called with no try/catch, and the `Disruptor` built in `createQueue()` (lines 166-172) registers no custom exception handler. So a throwable out of `BenchmarkCollector.collect()` → `consume()` hits LMAX's default `FatalExceptionHandler`, kills the event-processor thread, and is never written into `consumerFailure` — `checkConsumerFailure()` can't catch what nothing ever recorded. One scoping note worth being precise about: this exact gap (no custom exception handler on the Disruptor) already exists identically in production `IntermediateDisruptor`/`RecordEventHandler`, unmodified by this PR — so this isn't a new production behavior the PR introduces, it's the benchmark faithfully mirroring an exi sting production robustness gap. That said, I agree the harness itself should still fail fast here regardless of what production does, since "silently hang until the CI job timeout instead of reporting the regression" defeats the entire point of a regression-catching benchmark. Fixing it in `createQueue()`/`BenchmarkCollector` as you suggested is the right scope for this PR; improving production `IntermediateDisruptor`'s own exception handling would be a separate, unrelated issue. **Issue 2 (hardcoded `YieldingWaitStrategy` diverges from production) — not accurate as stated, based on what I verified.** I re-checked `TaskGroupWithIntermediateDisruptor.java:110-115` at the current head: production constructs its Disruptor with `new Disruptor<>(eventFactory, effectiveCapacity, DaemonThreadFactory.INSTANCE, ProducerType.SINGLE, new YieldingWaitStrategy())` — identical wait-strategy class, producer type, and thread factory to the benchmark's `createQueue()`. I'd already independently verified this match in my prior review round and just re-confirmed it holds at this head too, so there's no current fairness/drift bug. The "extract a shared factory method so the benchmark can never drift from production" suggestion is still a reasonable anti-drift guard for the future (if production's strategy ever changes, the benchmark would silently go stale), but I'd keep it filed as a non-blocking improvement, not a correctness fix for an existing mismatch. **Issues 4/5 (producer can block forever in `put()`/`received()` if the consumer dies mid-full-queue) — agreed, Medium, non-blocking.** This is a real TOCTOU window between `checkConsumerFailure()` and the blocking `received()` call, applies to both queue types, and matches your evidence. Reasonable hardening to queue for a follow-up. **Issue 8 (`setUp()` not exception-safe) — accurate on the JMH mechanics (no `@TearDown` on `@Setup` failure), but I'd temper the "contaminates subsequent trials" framing.** In practice a `@Setup` exception normally aborts that benchmark's run outright rather than continuing on to further trials in the same fork, so the blast radius is smaller than "leaks across trials" suggests. Still a reasonable low-cost defensive-cleanup addition, agreed non-blocking. **Where this leaves my recommendation**: I'm updating my own conclusion, not just replying past it. My last review's "no open issues / ready to merge" stands for everything I'd already checked, but Issue 7 is a real gap in my coverage — I didn't check `@Param` field visibility against JMH's generation requirements in either of my rounds, and if it does break generation, that's a must-fix, not a nice-to-have. Issue 1's family is also worth fixing given what this benchmark exists to catch. Since there's no new commit yet, I'm not posting a new formal review right now — once a commit lands addressing Issue 7 (and ideally the consumer-failure-capture gap), I'll do a full re-review of the new head rather than just diffing against this comment. -- 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]
