SEZ9 commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-5611960087
@DanielLeens — thanks for the candid follow-up, no apology needed; later passes catching what earlier ones missed is what layered review is for. I agree the durability-sample coverage and the non-atomic `get -> merge -> put` are carryover items rather than regressions from the latest commits, and thanks for doing the full re-review of the current head rather than only diffing the incremental commits. @Rangsh — thanks for the before/after runs (`75fd4ed` on run 34354309911 vs `c0d59da` on run 34354315647) and for flagging the CPU mismatch up front. Given the different hosts I'll treat these as observational, as you did; `runningJobGrowth` at count 1000 still around 19% CV on the after head is worth watching but isn't a blocker for the review items. I'll look into getting a same-worker `baseline -> PR -> PR -> baseline` run on the upstream repo since you can't dispatch there. On the review side, I can't tell from the thread which of the open items `c0d59da` addresses. Could you confirm per item: 1. Durability sample: restore per-iteration WAL-failure detection (or otherwise make a failed write-through append fail the measured `put`), and widen the sample to `loadAll` over the full batch rather than only the last key. 2. `storeFinishedPipelineMetrics`: either make the `get -> merge -> put` atomic on the IMap, or give a cluster-wide (not per-instance) argument for why the non-atomic path is safe and document that instead. 3. Docs: reflect the `runningJobGrowth` / `completedJobHistoryGrowth` methodology change in the benchmark docs, not only in Javadoc — this also ties to your point about the docs recommending same-worker comparisons. 4. Javadoc: trim the `storeFinishedPipelineMetrics` Javadoc to the contract and fix the "in memory" wording. 5. Tests: have `JobHistoryServiceFinishedMetricsTest` assert on the observable result rather than `never().computeIfAbsent`, and add the null-metrics and TTL-preserving cases. Once those are confirmed I'll do a final pass on the head. <!-- streview-comment:941 --> -- 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]
