DanielLeens commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5645188186
Confirming per item against `ab1538a9` (`ab1538a9c1241741940cc789e88fb79bf18df608`), with the code locations so you can check each one directly: - **F1 / F2**: `clearCoordinatorService()` now captures `JobHistoryService closingJobHistoryService = jobHistoryService;` at the top of teardown (`CoordinatorService.java`, right after the `coordinatorServiceCleared` CAS and before `pendingJobScheduleEpoch.incrementAndGet()`), and calls `closingJobHistoryService.close()` after the executor-termination wait. So it closes the instance owned by the coordinator generation being cleared, not whatever the mutable field points to at that moment. The field is deliberately not nulled: `close()` only deregisters the listener registrations, read methods keep working for callers already holding the reference, and `initCoordinatorService()` unconditionally constructs a new `JobHistoryService` on every activation. Both invariants are stated in comments at the getter and at the initialization site. `JobHistoryServiceListenerCleanupTest#testClearCoordinatorServiceDeregistersJobHistoryListeners` covers the production path on an isolated Hazelcast instance. - **F3**: yes. The constructor registers the three listeners inside a `try` block; on `RuntimeException` it calls `removeEntryListenerQuietly` for the state-map and metrics-map registrations already acquired (null-safe, so a failure on the first registration removes nothing) and rethrows the original exception. `JobHistoryServiceRegistrationTest` is parameterized over the failing registration index (each of the three), and separately verifies that a removal failure during rollback does not stop rollback of the remaining registration and that the caller still receives the original registration failure. - **F5**: yes. `removeEntryListenerQuietly` now has two catch blocks: `HazelcastInstanceNotActiveException` at `fine` (expected while the node is stopping), and any other exception at `warning` with the registration id and store name. It no longer swallows everything at a single level. - **F4 / F8**: in `JobHistoryServiceListenerCleanupTest#testCloseRemovesFinishedJobEntryListeners` every writer-created service is in try-with-resources (the positive control and each of the repeated create/close iterations), and `testClearCoordinatorServiceDeregistersJobHistoryListeners` shuts down its isolated Hazelcast instance in `finally`, so an assertion failure cannot leak listeners into the shared test node. The class is declared `extends AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest>`, not the raw type. - **F6 / F7**: `JobHistoryService implements AutoCloseable` with `@Override public void close()`, and `getEntryListenerRegistrationIds()` is package-private and annotated `@VisibleForTesting`. The "F2-F9" wording in my previous note was a typo on my side. The set is F1-F8 exactly as you raised it, all eight are in `ab1538a9`, and nothing is intentionally left as-is. -- 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]
