SEZ9 commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5707988103
Thanks for the rebase. I compared the current head `58157036bda4` against merge-base `b37af3a9bf634735cabe4908014c2d390b288558`: it is a mechanical rebase of `ccd7d928e59`, and the substance of `CoordinatorService.java`, `JobHistoryService.java` and `JobHistoryServiceTest.java` is unchanged from what was reviewed in 5136147528. No regressions, and `shutdown()` is still invoked from `clearCoordinatorService()` ahead of the steps that can throw, which is good. The open test items have not moved, though, so I'm keeping this at changes requested. What is still outstanding: 1. `testShutdownRemovesListeners()` calls `shutdown()` directly on `server.getCoordinatorService().getJobHistoryService()` and never drives `clearCoordinatorService()` with `resourceManager` / `eventProcessor` / `manager.close()` mocked to throw. Please add a test that exercises `clearCoordinatorService()` with one of those steps throwing and asserts all three listener UUIDs are removed. 2. That test should use an independently constructed `JobHistoryService` rather than the shared class-scoped instance that `testlistJobState` / `testGetJobStatus` depend on, so it doesn't tear down state the other tests rely on. Once those land I'm happy to do the next full pass promptly. If you see it differently, let me know here and we can work through it. <!-- streview-comment:1118 --> -- 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]
