SEZ9 commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5594760404
@DanielLeens thanks for pulling the actual `JobHistoryServiceTest.java` patch from `ccd7d928e59` and confirming both points independently — that settles it: - **Issue 3 (F3, ordering-under-throw test):** agreed. `testShutdownRemovesListeners()` exercises `jobHistoryService.shutdown()` directly, never goes through `clearCoordinatorService()`, and never makes `resourceManager` / `eventProcessor` / `manager.close()` throw. So the scenario F2 fixes (shutdown skipped when a later step throws) is still not verified by a test. Not done. - **Issue 6 (independent test instance):** agreed. The new test still reads `server.getCoordinatorService().getJobHistoryService()`, the shared class-scoped instance the other two tests depend on. Not done. And thanks for the correction on F1 — you're right, I was working from a stale view there. With the title changed to `[Fix][Zeta] Fix JobHistoryService IMap listener leak...` and the body referencing #11784 as "Part of" rather than "Closes", I consider F1 resolved and won't raise it again. On the #11809 overlap question: I have nothing beyond what's in this thread, so I can't confirm either way — that one stays on the author to answer. To summarise what's still outstanding on my side before this can move forward: 1. A test that drives `clearCoordinatorService()` with one of `resourceManager` / `eventProcessor` / `manager.close()` mocked to throw, asserting the three listeners are still deregistered. 2. A throwaway `JobHistoryService` instance for the new test so it doesn't share state with the existing tests in the file. 3. A plain yes/no on whether #11809 overlaps with this PR. Once those are pushed I'll do another full pass on the new head. <!-- streview-comment:901 --> -- 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]
