SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5390962824
Thanks @DanielLeens — the snapshot approach in c58e18421 looks like the right fix for the shutdown race (PR11809-F1). Capturing the instance into a local right after the guard and only closing that snapshot after the shutdown waits pins the cleanup to the coordinator generation being torn down. A few points from the earlier review are still open on this head: 1. **Stale closed reference (PR11809-F2)**: the mutable field can still point at a closed `JobHistoryService` after `clearCoordinatorService()` completes. Please either null the field out when appropriate, or document that every re-activation path must construct a new instance — right now correctness depends on that invariant silently. 2. **Partial-construction leak (PR11809-F3)**: if the second or third `addEntryListener` throws in the `JobHistoryService` constructor, the earlier registrations have no owner and `close()` can never remove them. Deregistering the already-captured UUIDs before rethrowing would close that gap. 3. **Test hygiene (PR11809-F4)**: the new test registers listeners on the shared node's cluster-wide IMaps without try/finally, so an assertion failure can leak live listeners into subsequent tests. 4. **Smaller items**: differentiating the log level in `removeEntryListenerQuietly` for non-shutdown failures (F5), implementing `AutoCloseable` (F6), adding a VisibleForTesting annotation on `getEntryListenerRegistrationIds()` (F7), and avoiding the raw `AbstractSeaTunnelServerTest` usage in the new test (F8) are all still outstanding — happy to have F5–F8 batched into one cleanup commit. You mentioned the Build is rerunning on this head — please share the result once it completes. Thanks for the quick turnaround on the race fix. <!-- streview-comment:526 --> -- 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]
