DanielLeens commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6266159602
Thanks @Rangsh for the per-item rundown — that's exactly the kind of mapping that makes a re-review fast, and it matches what I traced independently against `c0d59da`. On the one item @SEZ9 left open (F1 / the durability sampling window), here's my read, for the record: - I'm satisfied with the *shape* of the trade-off: first empty-pressure iteration plus trial tear-down, with the mid-trial full `loadAll` deliberately scoped out because it OOMs under `initialStoredJobCount=1000`. That's a real, measured constraint, not a shortcut. - What I'd still like is for the limitation to be written down rather than left implicit. As it stands, a reader of the fixture Javadoc can reasonably conclude that a WAL append failing mid-trial would be caught, and it would not be — it would only surface at trial tear-down. One or two sentences in the Javadoc (and, if you decide to keep any of it, in the benchmark docs) stating exactly which iterations are sampled and what the unsampled window can and cannot hide would close this cleanly. Same for the follow-up tracking issue for full-batch `loadAll` — a link in the thread or the Javadoc is enough to keep it from being lost. Two smaller notes so you have everything in one place: - **Same-runner comparison:** I can't dispatch `Benchmarks` on `apache/seatunnel` either, so the `baseline → PR → PR → baseline` shape @nzw921rx suggested isn't something I can kick off for you. Your own caveat about the two forks landing on different hosts (`AMD EPYC 9V74` vs `AMD EPYC 7763`) is the right call — treat those numbers as observational, which is how you've framed them. - **CV at `runningJobGrowth` / count 1000:** ~19% on the after head is on the noisy side, but with the host mismatch above I don't think it can carry a conclusion either way. It's worth watching, not worth blocking on. None of this is a regression from the recent commits — items 2, 4 and 5 in `ca82f95d3b`, item 3 in `909f878402`, and `c0d59da54` is a Javadoc-precision fix. I'd suggest folding the sampling-window note into the next push (or a Javadoc-only commit), and then this is ready for the final pass. -- 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]
