DanielLeens commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6359813281
Thanks @Rangsh and @SEZ9 for closing the loop on F6 with file and line evidence. I re-checked it myself on the unchanged head `b137173f7b7d1bb0a6889732401bb01a233585bb` (static read only; I did not run any tests or benchmarks): - `never().computeIfAbsent`: `JobHistoryServiceFinishedMetricsTest` contains no `computeIfAbsent` reference at all, so there is no assertion on a call-shape in that class. The success paths assert on the observable contract instead (the TTL-preserving `put(eq(1L), any(), eq(1440L), eq(TimeUnit.MINUTES))` verifications and the captured stored metrics). - Test name: the third success test at `:142` is `storeFinishedPipelineMetricsKeepsDistinctTaggedMeasurements`; `storeFinishedPipelineMetricsKeepsExistingMetricsForOtherTags` is not a method in this class at this head. The test methods in the file are `WritesOnceForNewJob` (`:92`), `MergesExistingFinishedMetrics` (`:112`), `RejectsNullMetrics` (`:135`) and `KeepsDistinctTaggedMeasurements` (`:142`), which matches what @Rangsh reported. So I agree F6 can be closed on this head. No new push has happened, so this is a thread follow-up and not another full review. Next steps, unchanged from my previous note: 1. Settle the current CV comparison (B = fixture only vs C = fixture plus `JobHistoryService`, both on the same resulting `dev` baseline with both SHAs recorded and raw samples compared on the same worker) before changing the sampling cadence; measure the requested cadence as a separate run afterwards. 2. The author-side source work is done; what remains is the benchmark evidence, a green `Build` on the final head, and a write-capable maintainer re-review. The earlier `CHANGES_REQUESTED` from @nzw921rx is still visible on GitHub and only a maintainer with write access can update or dismiss it; I cannot change it. Status evidence: head `b137173f7b7d1bb0a6889732401bb01a233585bb`, base `dev`, compare `diverged` (ahead 11, behind 356), mergeable state `blocked`. Thanks again for the careful evidence. -- 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]
