SEZ9 commented on PR #12453: URL: https://github.com/apache/seatunnel/pull/12453#issuecomment-5965669284
Thanks for the fix — wrapping the snapshot and assertions in the existing `await().atMost(10, TimeUnit.SECONDS).untilAsserted(...)` pattern is the right way to close the race between the synchronous `IMap` writes and the listener-fed counters, and like nzw921rx I think this is good to go in substance. A few small, non-blocking follow-ups from my earlier review: 1. **Comment accuracy:** the new comment says the logical counters are updated asynchronously, but that only applies to the five listener-fed `AtomicLong` gauges (the three checkpoint-monitor counters and the two running-job-metrics counters). The finished-job-record and connector-jar gauges are synchronous `IMap.size()`/`values()` reads. Keeping everything inside one `untilAsserted` block is fine, but please narrow the comment so it doesn't imply all ten gauges are eventually consistent. 2. **Contract documentation:** the eventual-consistency behaviour of the Hazelcast state-store counters is currently captured only in a test comment. Could you add a short note to the `CheckpointOverviewStateStore` Javadoc for the size()/count accessors stating that they are updated via entry listeners and are not read-your-writes? That way the next person hitting this flake finds the contract in the production interface. 3. **Shared helper:** `collectShouldTrackFinishedJobCleanupTotals` already uses the identical await-then-collect-then-assert shape. A tiny helper would remove the duplicated collector construction in both lambdas. 4. **Hoist the collector:** `new EngineStateStoreLogicalMetricExports(instance.node)` is re-instantiated on every Awaitility poll. Please create it once outside the lambda so the polled work is just `collect()`. All four are low severity and test/doc-only. If you can push those tweaks, I'll do a final pass. <!-- streview-comment:1490 --> -- 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]
