SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5789092332
Thanks for the detailed walkthrough against `ab1538a9`. **F1 / F2** — The reasoning makes sense: closing the captured local rather than the field in `clearCoordinatorService()`, and having a single assignment site in `initCoordinatorService()` that always constructs a fresh instance, addresses both points. I'll confirm against the diff before marking them resolved. One small ask for F1: since the safety argument rests on "close the snapshot, never the field", a one-line comment at the snapshot site would help keep a future refactor from collapsing it back into a direct field access. **F3** — Your comment appears to be cut off right after the `JobHistoryService.java:159-185` reference, so I can't evaluate this one yet. Could you post the rest? Specifically: if the second or third `addEntryListener` throws, does the constructor now remove the registrations made so far before rethrowing? **F4 / F5** — The heading mentions F1-F5, but F4 and F5 aren't in the visible text. Could you re-post them? For F4, whether the test now wraps listener registration in `try/finally` so an assertion failure doesn't leak live listeners into the shared node; for F5, whether `removeEntryListenerQuietly` now distinguishes shutdown-related failures from other removal failures, or at least logs them at a level that would surface a re-created leak. **F6-F8** (`AutoCloseable`, the `VisibleForTesting` annotation on `getEntryListenerRegistrationIds()`, raw `AbstractSeaTunnelServerTest`) — Not covered yet. A quick note on whether they're addressed in `ab1538a9` or deferred is enough. Once F3-F5 are visible and F6-F8 have a status, I'll do a final pass. <!-- streview-comment:1261 --> -- 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]
