DanielLeens commented on PR #12260: URL: https://github.com/apache/seatunnel/pull/12260#issuecomment-5633093703
Thanks for the independent re-trace, @SEZ9 — I verified both findings directly against the current head (`3cd44c2f542c`) rather than taking the descriptions at face value, and they both check out exactly as you describe: - **Issue 1** — confirmed. `SinkAggregatedCommitterTask.java:89` still declares `private Map<Long, Integer> checkpointBarrierCounter;` while the two sibling maps at lines 85/87 are both `ConcurrentMap<...>`, and `init()` (line 110) assigns a `ConcurrentHashMap` at runtime — so the weakly-consistent `keySet().removeIf(...)` at line 306 is safe today only by accident of the concrete type, not by the declared contract. Agreed this should be `ConcurrentMap<Long, Integer>` to match its siblings, with a short comment noting the sweep's reliance on the coordinator's single-pending-checkpoint invariant. - **Issue 2** — also confirmed. `testCheckpointBarrierCountersAreCleanedWithoutCommitInfo` (`SinkAggregatedCommitterTaskTest.java:137-153`) only seeds and asserts on `checkpointBarrierCounter`; it doesn't verify `aggregatedCommitter.commit(...)` is still invoked for the empty checkpoint or that `commitInfoCache`/`checkpointCommitInfoMap` stay empty. Your proposed additions (`verify(mockAggregatedCommitter).commit(Collections.emptyList())` plus the two `isEmpty()` assertions) would close that gap cheaply since `setUp` already stubs the mock. Both are accurate, and I'd rate them the same way you did — Low severity, non-blocking. They're hygiene/robustness improvements on top of a fix that's already correct and tested (the leak itself is real, the `<=` sweep is safe against the coordinator's actual single-pending-checkpoint behavior, and the core new test does prove the leak is gone). So this doesn't change my own approval — I'd be comfortable merging as-is and picking these up as a fast-follow, but if you'd rather see them landed in this PR first before your own approval stands, that's a reasonable bar to hold given how cheap both fixes are (a type declaration change and a few extra assertions in an existing test). -- 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]
