DanielLeens commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5370791448
Thanks for the second pass, @SEZ9 — I rechecked both new findings against the current head (`b0f7e78d6072`) and they hold up; I'm folding them into the blocking list. **Issue 2 (new, confirmed) — `jobHistoryService.shutdown()` is skipped whenever `eventProcessor.close()` throws.** Verified in `CoordinatorService.java:1172-1224`: `coordinatorServiceCleared.compareAndSet(false, true)` flips the guard to `true` at line 1174 *before* any of the cleanup work runs, then the event-processor close at lines 1211-1219 rethrows `SeaTunnelEngineException` on failure, aborting the method before it ever reaches `jobHistoryService.shutdown()` at lines 1221-1223. Because the CAS guard has already flipped, a subsequent `clearCoordinatorService()` call on the same instance short-circuits at line 1174-1176 and never retries — so a single event-processor-close failure permanently skips listener deregistration for that master-role-loss cycle, not just delays it. Agreed this is High severity and needs the shutdown call moved earlier (or into a `finally`), as you suggested. **Issue 4 (new, confirmed) — `removeEntryListenerQuietly` discards the boolean return of `IMap.removeEntryListener`.** Verified in `JobHistoryService.java:399-405`: the call result is never captured, so an already-removed or never-registered listener ID logs nothing. Agreed, Low severity but a legitimate diagnosability gap. Your Issue 1 and Issue 3 restate my Issue 1 (title/description/`Closes #11784` mismatch) and Issue 4 (zero test coverage) from my previous round — still open, no disagreement there. Updated blocking list for @zhang-arvin: (1) retitle/re-describe or actually implement #11784 and drop the auto-close reference, (2) fix the shutdown-skipped-on-exception ordering bug you found, (3) add the deregistration test. Issue 4 (boolean result) is a good non-blocking follow-up. Also still worth coordinating with #11809, which targets the same leak and doesn't have either of these two problems. -- 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]
