SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5659102017
Thanks for the follow-up — the rendering explanation makes sense, and the F6/F7/F8 summaries you repeated here are what I was looking for: `JobHistoryService` implementing `AutoCloseable` with an overriding `close()`, `getEntryListenerRegistrationIds()` package-private and marked as visible for testing, and the test class parameterized with itself rather than extending `AbstractSeaTunnelServerTest` as a raw type. I'll confirm those against the `ab1538a9` diff directly, but on paper F6–F8 look resolved. For the remaining points I only have the statement that they are all in `ab1538a9`, without the specifics, so a short note on each would speed up my re-review (otherwise I'll dig them out of the diff myself): - **F1** — how `clearCoordinatorService()` now avoids closing a `JobHistoryService` instance that a concurrent re-activation just created. - **F2** — whether the field is cleared (or otherwise made unusable) after `close()` so a stale, closed `JobHistoryService` cannot be picked up by a re-activation path that does not construct a fresh one. - **F3** — how the `JobHistoryService` constructor cleans up already-registered listeners if a later `addEntryListener` call throws. - **F4** — whether `testCloseRemovesFinishedJobEntryListeners` now unregisters its listeners in a `try/finally` so an assertion failure doesn't leak listeners into other tests sharing the node. - **F5** — what `removeEntryListenerQuietly` does now for non-shutdown failures versus the expected shutdown-time exceptions. No need to push anything else until I've finished going through F1–F8 on `ab1538a9`; if the five items above are covered, I expect this to be ready to approve. <!-- streview-comment:1043 --> -- 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]
