DanielLeens commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5421767106
Thanks for the detailed update, @SEZ9 — and no worries, the confusion was on the CI-trigger-commit noise, not on you. Agreed both blockers (the shutdown-ordering guarantee and the deregistration test) are still open on this head; I'll hold off on re-reviewing until a substantive commit lands. On your question: I'd move `jobHistoryService.shutdown()` to the very top of `clearCoordinatorService()`, right after the `coordinatorServiceCleared` CAS guard, ahead of the `eventProcessor.close()` try/catch — rather than a `finally` wrapping the whole method or per-step try/catch with an aggregated exception. `shutdown()` is already self-contained and exception-safe via `removeEntryListenerQuietly`, and it has no dependency on `eventProcessor` being closed first, so moving it to the front gives the same unconditional-execution guarantee as a `finally` block without restructuring the rest of the method. It also leaves the existing behavior of `eventProcessor.close()` failures propagating outward untouched, which looks intentional and shouldn't be swallowed. I'd avoid the per-step try/catch-with-aggregated-exception shape here — it adds real complexity for a method that currently only has one call needing this guarantee. Looking forward to the commit. -- 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]
