SEZ9 commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5421605057
@DanielLeens Thanks for taking another careful pass, and sorry for the confusion — you read it exactly right: `15b2ca68085` was only an empty "Trigger CI re-run" commit, so `git diff fe1b0fbdf19 15b2ca680` correctly shows no code change since the head you reviewed. Nothing substantive has landed yet, so both blockers are still open on my side. Status per your points: 1. **Shutdown ordering in `clearCoordinatorService()` — not done.** As you noted, `manager.close()` is unguarded and an exception from `processor.close()` will still skip `jobHistoryService.shutdown()`, defeating the very leak this PR fixes. I'll restructure so each close is individually guarded and `jobHistoryService.shutdown()` runs unconditionally (try/finally or per-step try/catch with aggregated logging), so no single failure can prevent listener deregistration. 2. **Test coverage — not done.** I'll add a test that exercises the register → `clearCoordinatorService()` → deregister lifecycle and asserts the entry listeners on `finishedJobStateImap`, `finishedJobMetricsImap`, and `finishedJobDAGInfoImap` are actually removed after a master-role loss, so the leak scenario is verified rather than assumed. 3. **`removeEntryListenerQuietly` swallowing the boolean result** — I'll check the return value of `IMap.removeEntryListener` and log a warning when it returns false, so silent removal failures are visible. 4. Glad Issue 1 (title/description mismatch) remains resolved with no regression. I'll push a real commit covering items 1–3 rather than another CI-trigger commit, and I'll ping you here once it's up. If you have a preference between try/finally versus per-step try/catch with a single aggregated exception at the end, let me know and I'll follow that shape. <!-- streview-comment:573 --> -- 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]
