SEZ9 commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-5628371885
Thanks @Rangsh for the detailed per-item rundown against `c0d59da`. 1. **Durability sample (F1 / F3)** — Sampling at the first empty-pressure iteration plus trial tear-down (`ca82f95` / `bf1110b` / `3794d03`) is a clear improvement over last-batch-only, and the OOM constraint under `initialStoredJobCount=1000` is a fair reason not to run a full `loadAll` mid-trial. One gap I'd like to reconcile before closing: iterations between the first and the last still go unsampled, so a WAL append that starts failing mid-trial would only surface at tear-down. Could you note that limitation in the Javadoc/docs alongside the coverage description, or explain why it isn't a concern here? Treating the full-batch `loadAll` widening (F3) as a follow-up is fine with me — please open a tracking issue so it isn't lost. 2. **`storeFinishedPipelineMetrics` atomicity (F2)** — Wrapping `get → merge → put` in `finishedJobMetricsImap.lock(jobId)` / `unlock(jobId)` with `try`/`finally` is the right fix. I'll verify it in the `ca82f95` diff; marking as pending verification for now. 3. **Docs (F4)** — Thanks for adding the growth methodology in `909f878`. Pending verification in the diff as well. Your comment looks cut off at "Same-worker comparisons remain the recommended path in the Benchmarks" — feel free to finish the thought if there was more. Two items from the earlier review that weren't covered in your comment: - **F5 — `storeFinishedPipelineMetrics` Javadoc**: with the merge now lock-protected, has the Javadoc been updated to describe the current contract rather than the historical per-JobMaster-instance rationale, and to drop the "in memory" wording? If one of the listed commits already did this, a pointer is enough. - **F6 — `JobHistoryServiceFinishedMetricsTest`**: has the test been reworked to assert on the resulting IMap state (merged metrics, TTL retained, null metrics handled) instead of `never().computeIfAbsent`? If not, I'd like that covered before merge. Once F5/F6 are addressed and I've confirmed the above in the diff, I'm happy to approve. <!-- streview-comment:950 --> -- 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]
