SEZ9 commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-6105290808
Thanks @zhang-arvin for the follow-up and the clear walkthrough of the new test. **F3 (listener-deregistration lifecycle untested) — closed.** `JobHistoryServiceTest#testShutdownRunsBeforeThrowingCleanupStep` on head `94ada9982` drives `clearCoordinatorService()` itself with a later cleanup step throwing, and asserts both that the exception propagates and that `shutdown()` was still invoked. Together with `#testShutdownRemovesListeners`, that covers the regression we were worried about. Noted that `ccd7d928e -> 58157036b` was rebase-only and that the PR's own file content is unchanged apart from the new test. **F2 (`shutdown()` skipped when a later close throws) — looks resolved.** With `jobHistoryService.shutdown()` now running before the cleanup steps that can throw, the leak-on-throw path is gone, and the new test pins that ordering. The one item still open from my latest review is the shared-instance concern raised earlier, which matters a bit more now that the new test is destructive. Once that is addressed I'm happy to take another look. <!-- streview-comment:1674 --> -- 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]
