DanielLeens commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5553493462
Thanks for the quick and thorough response, @SEZ9 — and glad the indentation slip is confirmed as a simple fix. On your two open questions: **Issue 3 (test scope):** I'd like coverage that actually exercises the ordering guarantee through `clearCoordinatorService()` itself, not just a direct call to `shutdown()`. A unit test asserting `shutdown()` invokes `removeEntryListener` with the captured UUIDs is good baseline coverage, but it doesn't touch the specific scenario this PR exists to fix: listener deregistration must still happen even when a step ahead of it (e.g. `eventProcessor.close()`) throws. Please mock `resourceManager`/`eventProcessor` so `close()` throws, invoke `clearCoordinatorService()`, and assert the three `IMap` listeners are still removed. That's the one scenario that's taken several rounds of manual inspection to re-confirm each time this method changes (including the recent rebase) — an automated test is what keeps it settled going forward. **Issue 4 (boolean result):** A warning log is sufficient — no need for anything stronger. Capturing and logging the `false` return from `removeEntryListener` closes the diagnosability gap; it doesn't need to throw or escalate. I'll hold off on re-reviewing until the next commit lands with the indentation fix, the description update, and the F3/F4 changes — happy to take another pass as soon as it's up. One more thing that's come up a couple of times in this thread and still hasn't been addressed: #11809 targets the exact same listener leak and is still open (not merged). Could you confirm whether the two PRs have been compared for overlap? If one already covers the other's fix, it'd be worth consolidating so we don't land two competing changes to the same code path. -- 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]
