SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-6552677114
Thanks for the follow-up. Before signing off I want to verify the earlier points against the current diff rather than rely on confirmation alone. Could you point me to the relevant changes for each? **F1 / F2 (coordinator ownership):** Where is the owned `JobHistoryService` captured right after the clear guard and that captured instance closed, rather than re-reading the mutable field? Since the field may still reference a closed instance afterwards, a short comment at the single assignment site noting that re-activation must always construct a fresh instance would help preserve that invariant. **F3 (partial construction):** Where are already-acquired registration IDs rolled back before the original failure is rethrown? Please also make sure a failure during the rollback removal itself doesn't mask the original exception (log it and still rethrow the original). **F4 (test listener leak):** Does the new close test wrap its registrations in try/finally so an assertion failure doesn't leave live listeners on the shared node's IMaps? **F5 (removal logging):** Which failures are logged at fine level versus warning in the quiet removal helper? Please make sure the warning path includes the map name / registration ID so a re-created leak is diagnosable. **F6 / F7 / F8:** Does `JobHistoryService` implement `AutoCloseable`, is `getEntryListenerRegistrationIds()` annotated as visible-for-testing, and does the new test no longer extend `AbstractSeaTunnelServerTest` as a raw type? Once I can see these in the diff, I'm happy to approve. <!-- streview-comment:2186 --> -- 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]
