SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5409353881
Thanks for the thorough re-verification pass, @DanielLeens — consolidating it into one comment was the right call. Agreed on **F1**: snapshotting the field into a local right after the CAS guard in `clearCoordinatorService()` and closing that local pins cleanup to the correct coordinator generation. I consider that resolved at head `c58e1842`. On **F2**, your trace appears cut off mid-sentence, so I can't see your full conclusion — could you re-post the rest? My ask stands regardless: even if every current re-activation path constructs a fresh `JobHistoryService`, leaving a closed instance referenced by the field keeps correctness implicit. Nulling the field after closing the local (or otherwise making the invariant explicit) would be cheap insurance. Remaining items at the current head: - **F3 (MEDIUM)**: guard partial construction in the `JobHistoryService` constructor — if the second or third `addEntryListener` throws, deregister the earlier registrations before rethrowing, otherwise `close()` can never reach them. - **F2 (MEDIUM)**: as above, make the closed-instance invariant explicit in `CoordinatorService`. - **F4**: wrap the listener registrations in `testCloseRemovesFinishedJobEntryListeners` in try/finally so an assertion failure doesn't leak live listeners into subsequent tests on the shared node. - **F5**: in `removeEntryListenerQuietly`, distinguish expected shutdown-time failures from genuine removal failures instead of swallowing everything at a single warn level. - **F6/F7/F8 (style, quick)**: implement `AutoCloseable` on `JobHistoryService`, annotate `getEntryListenerRegistrationIds()` as visible-for-testing, and parameterize the raw `AbstractSeaTunnelServerTest` usage in the new test. The core fix is in good shape — F2 and F3 are the two I'd like addressed before merge; the rest are small follow-ups that can land in the same push. Thanks again for the careful work here. <!-- streview-comment:554 --> -- 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]
