SEZ9 commented on PR #12391: URL: https://github.com/apache/seatunnel/pull/12391#issuecomment-5964424977
@SeaSand1024 thanks for the CI breakdown on tip `2cc06fc43`. Good to see `all-connectors-it-5` green on both JDKs after the dev sync, and the attempt-2 re-run clearing `unit-test (11, ubuntu-latest)`, `all-connectors-it-2 (8, ubuntu-latest)` and `oracle-cdc-connector-it (8, ubuntu-latest)` — the attribution to the savepoint test, Maven dependency resolution and the wrapper download 404 looks like unrelated flakiness, so nothing there blocks this PR. Since the sync contained no close-path edits, the earlier review points are still open. What's still needed in `SeaTunnelTask.java`: 1. **F1 – double-close / idempotency:** when a single lifecycle `close()` fails on the normal CLOSED/CANCELLING path, the method still completes the pass and rethrows, so the `BlockingWorker` fallback closes every lifecycle (including the clean ones) again. Please make `close()` idempotent (or track which lifecycles already closed) so the fallback doesn't re-close them. 2. **F2 – `catch (Throwable)` in both teardown loops:** this swallows fatal JVM errors (OOM, ThreadDeath, StackOverflowError) and keeps calling connector `close()` after an OOM, while `BlockingWorker` downgrades the error to a `severe` log. Please narrow to `Exception` (or rethrow `Error`s immediately). 3. **F5 – `super.close()` catch + `allCycles` guard:** the widened `catch (Throwable)` around `super.close()` can only collect an `Error` in practice, and `allCycles` is still dereferenced unguarded right after, so an `init()` failure before `allCycles` is assigned still NPEs on the fallback close path. A null check there would be enough. 4. **F4 – Javadoc + log context:** the Javadoc throws clauses no longer reflect the actual control flow, and the per-lifecycle error log lacks task/lifecycle identifiers, so each failure is logged twice (here and in `BlockingWorker`) with no way to correlate them. Please update the Javadoc and add task id + lifecycle name to the log line. On the test side (**F3**, `SeaTunnelTaskStateTest.java`): the new tests exercise the loop in isolation but not the two paths this PR actually changes end-to-end — the `super.close()` failure branch and the `BlockingWorker` fallback that now absorbs unchecked close failures. A test for each would let us verify F1/F5 behave as intended. Once those land I'm happy to take another look. No need to re-run the full CI matrix before then — a green `unit-test` on the follow-up commit is sufficient. <!-- streview-comment:1479 --> -- 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]
