DanielLeens commented on PR #12248: URL: https://github.com/apache/seatunnel/pull/12248#issuecomment-5633092606
Hi @nzw921rx, thanks for engaging with this so thoroughly — these are exactly the right questions to ask about a diagnostics feature before it becomes permanent maintenance surface. To your direct question first: no, this report on its own cannot determine the *cause* of a discrepancy, and it isn't meant to. Looking at the implementation and the docs added in this PR (`docs/en/engines/zeta/benchmark.md`), the per-fork breakdown is explicitly framed as descriptive evidence, not a regression gate or a root-cause verdict. What it changes is the first triage step: today, when Score/Error/CV alone look suspicious, there's no way to tell — without pulling the raw JSON yourself — whether the variance is spread evenly across forks (consistent with something environmental affecting the whole run) or concentrated in one fork (consistent with a noisy neighbor on that specific fork). The table answers "is this within-fork noise or between-fork drift?" in one glance; it doesn't answer "why" — `benchmarks_diagnostics.yml` is still the next step once that first question comes back "yes, something looks off." On your broader point about `benchmarks_core` having too many scenarios, and whether `Pipeline.sourceTransformSink` / `IntermediateQueue.disruptorRecordHandoff` actually validate this feature's premise — that's a fair challenge, and it's genuinely a separate proposal (curating the long-term report) from what this PR does. To be precise about scope: this PR doesn't add, remove, or reorder any benchmark in the long-term report, and it doesn't change the existing Score/Error/CV output at all — I checked this myself in review, the new test `test_fork_diagnostics_distinguish_within_and_between_fork_variation` asserts byte-identical `jmh_report_lines()` output for two datasets with different fork boundaries. The fork table is an additional, collapsed-by-default section that only shows up when `fork_samples` data exists, so even if `benchmarks_core` does get trimmed down to fewer, more stable metrics, that's orthogonal and wouldn't conflict with merging this — the new section just travels with whichever benchmarks remain. Your empirical observation that you haven't seen a single-fork-high case in your own monitoring is useful signal, though, and it's really a question for @FenjuFu rather than something I can answer from the diff alone: whether this was motivated by a specific past investigation (the PR references #12086) where a one-fork-high situation actually occurred, since that would be the strongest justification for keeping the maintenance cost either way. I'd encourage continuing this thread with them directly on that point. -- 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]
