DanielLeens commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-5645334501
Thanks @Rangsh — confirmed, that matches what I verified independently: F6 in `ca82f95` covers the observable-contract assertion (single `put` + `lock`/`unlock`), the null-metrics rejection, and the TTL-preserving path. Nothing else open from the review side on this head. One correction to my last review, though: I noted `Build` was still `in_progress` on `c553a8c04` at review time and said I had no reason to expect a real failure since the commit was comment-only. That turned out to be wrong — the run has since completed, and it's a real, PR-caused failure, not noise: `Run / benchmark-test` fails on **both** JDK 8 and JDK 11 (fork run https://github.com/Rangsh/seatunnel/actions/runs/34558258789) at the `Build benchmark dependencies` step, with `spotless:check` rejecting `IMapJobGrowthBenchmarkWorkload.java`: ``` [ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.29.0:check (spotless-check) on project seatunnel-benchmarks: The following files had format violations: [ERROR] src/main/java/org/apache/seatunnel/benchmark/storage/imap/IMapJobGrowthBenchmarkWorkload.java [ERROR] @@ -292,7 +292,8 @@ ... [ERROR] -········//·FileMapStore.loadAll·replays·the·full·WAL·into·heap.·Under·initialStoredJobCount=1000·that [ERROR] +········//·FileMapStore.loadAll·replays·the·full·WAL·into·heap.·Under·initialStoredJobCount=1000 [ERROR] +········//·that ``` This is exactly the new comment line this round's commit (`c553a8c04`) added on `shouldSampleGrowthDurability()` — it's too long for the enforced formatting and needs to be re-wrapped. Running `mvn spotless:apply -pl seatunnel-benchmarks` locally and pushing the result should resolve it directly (deterministic formatting failure, not flaky — will fail again on rerun without the fix). The other three failing jobs on this same run (`all-connectors-it-6`, `all-connectors-it-7`, `elasticsearch-connector-it`) don't look related to this PR's diff — this PR only touches `seatunnel-benchmarks` and `JobHistoryService`/its test, no connector code — so I'd treat those as pre-existing/environmental for now, but worth re-confirming they're still green once you push the spotless fix and CI reruns. Once `mvn spotless:apply` is pushed and `Build` is green on the new head, I don't have anything else open — this becomes ready to merge from my side, same as my last review's conclusion otherwise. As before, I only have comment-only review rights here, so the formal approval/merge (and re-confirming @nzw921rx's now-stale `CHANGES_REQUESTED`) still needs a maintainer with write access. -- 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]
