SEZ9 commented on PR #12391: URL: https://github.com/apache/seatunnel/pull/12391#issuecomment-6307735508
Thanks for the follow-up work here. Looking at the current head (2a5e21fe, a merge of `dev`; the rest of the diff matches 6ad74f1f), here is where I stand on my earlier points: 1. **Double close / idempotency** – The `AtomicBoolean closed` latch with `compareAndSet(false, true)` plus the `try/finally` around `close()` in the CLOSED and CANCELLING branches addresses this. Resolved from my side. 2. **`catch (Throwable)` swallowing fatal errors** – Stopping the loop early for `VirtualMachineError` / `ThreadDeath` and letting an `Error` take priority over an `Exception` covers the concern in `SeaTunnelTask`. One remaining question: does the `BlockingWorker` fallback rethrow (rather than only log) when the propagated failure is an `Error`, so a fatal error during close is not downgraded to a log line? 3. **Test coverage** – With `closeSuper()` extracted, could you point me to (or add) tests that exercise (a) `closeSuper()` throwing while the lifecycle loop still runs, and (b) the `BlockingWorker` fallback path hitting the already-closed latch and absorbing an unchecked close failure? 4. **Javadoc / log context** – Logging with task location + lifecycle name resolves the correlation half. Please double-check that the throws documentation on `close()` and `closeSuper()` matches the new flow (first failure rethrown, `IOException` as is, other throwables rethrown unwrapped, remaining failures attached as suppressed). 5. **`allCycles` null guard** – Is `allCycles` now guarded before the iteration for the case where `init()` failed before it was assigned? If not, a simple null check (or treating it as an empty list) in `close()` would close this one out. Once items 2–5 are confirmed or addressed, I think this is good to go. <!-- streview-comment:1920 --> -- 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]
