SEZ9 commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5381239179
Thanks for the quick turnaround, @zhang-arvin! A few things before we can close this out: 1. **Please push the changes (or point us to the new head).** The latest commit referenced in this thread is `b0f7e78d6072`, which is the head @DanielLeens verified the findings against. I can't see the fixes you describe yet — once the new commits are up, we'll re-review. 2. **Shutdown ordering fix.** For the `clearCoordinatorService` issue, please confirm how you fixed it: the key requirement is that `jobHistoryService.shutdown()` runs even when `eventProcessor.close()` throws (e.g., moving it earlier or into a `finally`), since the CAS guard flips before cleanup and a single failure otherwise permanently skips listener deregistration for that cycle. 3. **Test coverage is still a blocking item.** Your summary covers the ordering fix, the `removeEntryListener` result handling, and the title/description update, but not the deregistration test from the blocking list. We need a test that verifies listeners are actually removed across a master-role switch — please add one or let us know if it's included in your pending push. 4. **Title/description scope.** Thanks for updating it — please also make sure the auto-close reference to #11784 is dropped, since the time-range metrics history feature isn't implemented here. Lastly, as @DanielLeens noted, it's worth coordinating with #11809 since it targets the same leak — please check for overlap so we don't land conflicting fixes. Happy to re-review as soon as the commits land! <!-- streview-comment:454 --> -- 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]
