SEZ9 commented on PR #12391: URL: https://github.com/apache/seatunnel/pull/12391#issuecomment-6050917253
@SeaSand1024 thanks for the follow-ups on `563fd4e8a` and `6ad74f1fb`. **F3:** dropping the test-thread classloader assert and documenting why in the comment sounds right. I'll verify the end-to-end NCDFE coverage (`testCloseAttemptsEveryCycleWhenCycleThrowsNoClassDefFoundError`), the kept `super.close()` failure test, and `testBlockingWorkerFallbackCloseWhenCallAndCloseThrowRuntimeException` in the diff before resolving. If you can share the name of the `super.close()` failure test, that will speed it up. Remaining points I'll check against the diff; pointers are welcome but I'll resolve each based on the code and tests rather than on the comment thread: - **F1 (double close / idempotency):** since `close()` completes the full pass and then rethrows, lifecycles that closed cleanly can be closed again by the BlockingWorker fallback. Does the non-transient `closed` latch short-circuit that second pass, and which test exercises it? If it does not, please explain why the second pass is safe. - **F2:** the `VirtualMachineError` / `ThreadDeath` early-stop addresses the lifecycle loop. For the fallback, your note says BlockingWorker is an unchanged `catch (Exception)`, whereas F2 was raised because the fallback catches `Throwable` and downgrades a fatal error to a `severe` log line. Those two readings conflict, so I'll re-read the fallback catch in the diff before marking that half resolved; if you believe F2 misread it, point me to the relevant lines. - **F4:** does the per-lifecycle error log now carry task/lifecycle context so it can be correlated with the `Close task error` line from BlockingWorker? - **F5:** is `allCycles` still dereferenced unguarded after the `super.close()` catch? If an `init()` failure before `allCycles` is assigned still NPEs in the fallback close path, a null guard plus a small test would close this out. I'll follow up once I've gone through the diff. <!-- streview-comment:1586 --> -- 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]
