SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5643147762
Thanks for the update. Good to see the fork Build run for head `ab1538a9` (run 34322099296) reported as successful. Before a final pass, I'd like to close the loop on my earlier findings. The note above refers to an F2-F9 range, while the set I raised was F1-F8, so I don't want to assume which items were addressed. Could you confirm how each of these stands on `ab1538a9`: - **F1 / F2**: does `clearCoordinatorService()` now close the specific `JobHistoryService` instance it owns rather than whatever the mutable field currently points to, and is the closed instance no longer left referenced by the field (or is every re-activation path guaranteed to construct a new instance)? - **F3**: if the second or third `addEntryListener` in the `JobHistoryService` constructor throws, are the earlier registrations removed before the exception propagates? - **F5**: does `removeEntryListenerQuietly` now distinguish non-shutdown removal failures instead of swallowing everything at a single warning level? - **F4 / F8**: are the listener registrations in the new cleanup test wrapped in try/finally so an assertion failure doesn't leak listeners into the shared test node, and is `AbstractSeaTunnelServerTest` extended with its type parameter instead of as a raw type? - **F6 / F7**: does `JobHistoryService` implement `AutoCloseable`, and is the test-only accessor `getEntryListenerRegistrationIds()` marked as visible for testing? If all of these are already in `ab1538a9`, a short "done" list per item is enough and I'll re-review. If any are intentionally left as-is, please say which and why so we can settle it here. <!-- streview-comment:988 --> -- 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]
