Rangsh commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6306736239
@SEZ9 status against `b137173f7`, no new push. **F4 — agreed, closed.** Javadoc stays the source of truth. No note in `docs/en|zh/engines/zeta/benchmark.md`. **F1 / F3 — unchanged by `b137173f7`.** `git show b137173f7` only edits comments in `IMapJobGrowthBenchmarkWorkload.java`. It does not change the trial-tear-down sample or the last-key-only check. - **F1:** the behavior on this head is already the one described in the class Javadoc and on `verifyGrowthPhase()`. Only the first `initialStoredJobCount=0` iteration durability-samples inside `verifyGrowthPhase()`. Every later iteration is resident-only, and `initialStoredJobCount=1000` is never sampled there. `tearDown()` always calls `verifyGrowthBatchDurability()`. A missing resident entry is still caught every iteration. A write-through WAL append that starts failing after the first sample is not caught until trial tear-down. I am not changing that behavior in this push. If you still want it resolved before merge, please say what change you want on top of this, and I will do it as a separate commit after the current same-worker CV comparison is settled. - **F3:** still last-key-only, intentional. `verifyRunningJobBatchDurability()` and `verifyCompletedJobBatchDurability()` call `loadAll(Collections.singleton(key), true)` for the last key only. The other keys are resident checks. One `loadAll` over the whole final batch is #12266, not this PR. I am not widening it here, because that changes the fixture under the open CV comparison. **F2 / F5 / F6 — already done in `ca82f95d3b`, not still open as originally written.** Those files being byte-identical at `b137173f7` means they were not changed again. It does not mean the fixes are absent. - **F2:** `storeFinishedPipelineMetrics` (`JobHistoryService.java:278-288`) locks `finishedJobMetricsImap` on `jobId`, then does `get → merge → put` inside `try/finally`, and unlocks in `finally`. That is the IMap key lock, not a per-`JobMaster` monitor. `JobMaster.savePipelineMetricsToHistory` also holds `metricsLock`, but this method does not depend on that lock. - **F5:** the current Javadoc (`JobHistoryService.java:267-277`) states the contract: lock `jobId`, merge into the current value, one TTL `put`. It does not describe a historical per-instance rationale, and it does not say "in memory". - **F6:** `JobHistoryServiceFinishedMetricsTest` asserts the stored `put`, not `never().computeIfAbsent`. Merge is `storeFinishedPipelineMetricsMergesExistingFinishedMetrics` (`:112-131`). Null is `storeFinishedPipelineMetricsRejectsNullMetrics` (`:135-138`). TTL `put(..., 1440, MINUTES)` is asserted on the new-job, merge, and distinct-tag tests. Could you re-read F2/F5/F6 against `ca82f95d3b` as contained in `b137173f7`? Unless that pass still finds a gap, I will not push a code change for them. -- 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]
