DanielLeens commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5645098932
Thanks for catching the numbering gap — that's on me, not a discrepancy in the fix itself. My 09-09 write-up covered F2 through F8 and dropped F1 off the list by mistake; it's addressed in the same commit. I went back through `ab1538a9` line-by-line just now to confirm each item against current source rather than re-asserting from the commit message: - **F1**: Fixed. `clearCoordinatorService()` captures the generation-owned instance into a local before any shutdown waits run (`JobHistoryService closingJobHistoryService = jobHistoryService;`, `CoordinatorService.java:1304`) and calls `.close()` on that local, not on whatever the field currently holds (`:1352-1353`). `initCoordinatorService()` still unconditionally constructs a fresh instance on every activation with no reuse branch (`:632-646`), so a re-activation racing shutdown can never have its listeners closed by the outgoing generation's cleanup. - **F2**: The field is deliberately left non-null after close, same as before, so existing read-path callers keep working without a null-check contract change. Since F1 means `close()` never touches whatever the field points to at call time, and every activation is a guaranteed fresh construction, the "closed instance still reachable via the field" risk no longer has a path to actually matter in the current code. - **F3**: Fixed. The constructor wraps the three `addEntryListener` calls in try/catch; on a `RuntimeException` from the 2nd or 3rd call it removes whatever was already registered via `removeEntryListenerQuietly` and rethrows the original exception (`JobHistoryService.java:162-185`). - **F4 / F8**: Fixed. `testCloseRemovesFinishedJobEntryListeners` now uses try-with-resources for both the positive-control instance and each loop-created instance (`JobHistoryServiceListenerCleanupTest.java:78`, `:90`); the class extends `AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest>`, not the raw type (`:58-59`). - **F5**: Fixed. `removeEntryListenerQuietly` now catches `HazelcastInstanceNotActiveException` separately at `fine` for the expected-shutdown case and falls back to `warning` with the registration id for anything else (`JobHistoryService.java:480-493`). - **F6 / F7**: Fixed. `JobHistoryService implements AutoCloseable` (`:71`); `getEntryListenerRegistrationIds()` carries `@VisibleForTesting` (`:226-227`). All eight are in on `ab1538a9`, verified against source just now rather than carried forward from the commit message. Nothing left open on my side — over to you for the re-review. -- 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]
