SEZ9 commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6331099750
Thanks @Rangsh for the point-by-point status against `b137173f7`. **F4** — agreed, closed. Javadoc stays the source of truth; no note needed in `docs/en|zh/engines/zeta/benchmark.md`. **F2 / F5 / F6** — understood that these landed in `ca82f95d3b` and were not touched again in `b137173f7`. I'll verify them against the `ca82f95d3b` diff rather than closing on the description alone: - F2: locking `finishedJobMetricsImap` on `jobId` around `get → merge → put` with unlock in `finally` (`JobHistoryService.java:278-288`) is the IMap key lock I was after. If the diff matches, closed. - F5: a Javadoc stating lock `jobId` → merge into current value → one TTL `put`, with no historical rationale and no "in memory" wording (`JobHistoryService.java:267-277`), resolves this once confirmed in the diff. - F6: asserting the stored `put` rather than `never().computeIfAbsent` addresses the first half. Your comment appears to have been cut off in the F6 section, so I can't see the rest — could you confirm whether the null-metrics and TTL-preserving paths are also covered in `JobHistoryServiceFinishedMetricsTest`, and point me at the relevant test methods? **F3** — accepted as intentional for this PR. Widening to one `loadAll` over the whole final batch belongs in #12266; I won't ask for it here given the open CV comparison. **F1** — yes, I'd still like this resolved before merge, and a separate commit after the same-worker CV comparison is settled works for me. The concrete change: have `verifyGrowthPhase()` durability-sample on every iteration (so `initialStoredJobCount=1000` is sampled too), not only the first `initialStoredJobCount=0` iteration, keeping `tearDown()` → `verifyGrowthBatchDurability()` as-is. That way a write-through WAL append that starts failing after the first sample is caught at that iteration rather than at trial tear-down. If per-iteration sampling measurably perturbs the fixture, let me know and we can discuss a cheaper cadence. Remaining asks: 1. F2 / F5: I'll confirm against the `ca82f95d3b` diff. 2. F6: confirm null-metrics and TTL-preserving coverage. 3. F1: per-iteration durability sample in `verifyGrowthPhase()` as a follow-up commit once the CV comparison is done. F3 and F4 are closed from my side. <!-- streview-comment:1931 --> -- 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]
