DanielLeens commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5661717266
Specifics on F1-F5, all against `ab1538a9`: **F1 — `clearCoordinatorService()` vs. a concurrent re-activation.** [`CoordinatorService.java:1304`](https://github.com/apache/seatunnel/blob/ab1538a9c1241741940cc789e88fb79bf18df608/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java#L1304): the method captures the field into a local, `JobHistoryService closingJobHistoryService = jobHistoryService;`, *before* the `executorService.awaitTermination(20, TimeUnit.SECONDS)` wait at line 1335 (which is the window where a concurrent re-activation could run). The actual `.close()` call at line 1352-1353 is made on that captured local, not on the field. So if a concurrent `initCoordinatorService()` reassigns the `jobHistoryService` field to a freshly-constructed instance while this method is still waiting on `awaitTermination`, the close at the end of `clearCoordinatorService()` still targets the *old* generation's instance it snapshotted, never the new one. **F2 — field left non-null after close, and why that's safe.** Confirmed: the `jobHistoryService` field is deliberately not nulled — the comment directly above the close call ([line 1350-1351](https://github.com/apache/seatunnel/blob/ab1538a9c1241741940cc789e88fb79bf18df608/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java#L1350-L1351)) says "The instance itself is kept because read paths may still use it." This is safe because there is exactly one assignment site for the field in the whole class — [`initCoordinatorService()` at line 632-633](https://github.com/apache/seatunnel/blob/ab1538a9c1241741940cc789e88fb79bf18df608/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java#L632-L633), which every re-activation calls and which unconditionally constructs a brand-new `JobHistoryService` with its own comment ("Never reuse the previous history view"). The re is no re-activation path that reuses the field instead of constructing fresh, so a stale-but-closed instance can only ever be reached by requests that already held a reference from before the close — which is exactly the intended "reads still work" behavior, not a re-activation path. **F3 — constructor rollback on a later `addEntryListener` failure.** [`JobHistoryService.java:159-185`](https://github.com/apache/seatunnel/blob/ab1538a9c1241741940cc789e88fb79bf18df608/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java#L159-L185): the three `addEntryListener` calls (state, metrics, DAG info, in that order) assign into local variables first, and a `catch (RuntimeException e)` around all three calls unwinds via `removeEntryListenerQuietly` for the state and metrics ids before rethrowing. Because `removeEntryListenerQuietly` no-ops on a null id, this correctly handles every failure point: if the 2nd call throws, only the 1st (state) registration gets rolled back; if the 3rd (DAG info) call throws, both the 1st and 2nd are rolled back. A failed constructor never publishes the instance, so nothing else can observe the partially-rolled-back state in between. **F4 — assertion-failure safety in `testCloseRemovesFinishedJobEntryListeners`.** [`JobHistoryServiceListenerCleanupTest.java:78` and `:90`](https://github.com/apache/seatunnel/blob/ab1538a9c1241741940cc789e88fb79bf18df608/seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java#L78): both blocks in this test use Java try-with-resources (`try (JobHistoryService x = newJobHistoryService())`) rather than a bare local + explicit `finally`. Since `JobHistoryService implements AutoCloseable`, the JVM guarantees `close()` runs during exit of each try block on any exception path, `AssertionError` included, without needing an explicit `try/finally` — so a failed `Assertions.assertX` inside either block still deregisters that iteration's listeners before propagating. **F5 — `removeEntryListenerQuietly` failure handling.** [`JobHistoryService.java:475-494`](https://github.com/apache/seatunnel/blob/ab1538a9c1241741940cc789e88fb79bf18df608/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java#L475-L494): `HazelcastInstanceNotActiveException` is caught separately and logged at `.fine` — treated as the expected case where the node is already shutting down and its event service owns the remaining cleanup. Any other `Exception` is caught and logged at `.warning`, i.e. treated as unexpected and worth surfacing, but still swallowed so listener cleanup can never break the master-switch or shutdown flow itself. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
