SEZ9 commented on PR #12391: URL: https://github.com/apache/seatunnel/pull/12391#issuecomment-6029480373
Thanks for the heads-up on the classloader assert — agreed. Since the Awaitility `untilAsserted` block runs on the test thread, it cannot observe the worker thread's context classloader, so it does not actually verify the nested-`finally` restore in `BlockingWorker`. I also agree the production restore is correct as written on `563fd4e8a`, so this is not a merge blocker. Rather than dropping the assert outright, my preference would be to have the test capture the worker thread's context classloader from inside the close path and assert on it after the worker has finished. If that is awkward with the current test seam, dropping it with a short comment explaining why is fine too. Either way, a follow-up is acceptable — just link it from this thread so it doesn't get lost. On the rest of your `563fd4e8a` summary: the Error-policy split (full pass for non-fatal Errors, early-stop only for `VirtualMachineError` / `ThreadDeath`, prefer `Error` over earlier `Exception`), the nested `finally` restore, the end-to-end NCDFE coverage plus the retained `super.close()` failure test, and the Javadoc updates all line up with what was asked. The `SplitClusterFaultToleranceIT` cancel-vs-FAILED failure you described on `877056bf1` / run `37097098821` reads as outside the close-path changes here, so I'm fine not folding it into this tip. Remaining asks from my side: (1) the classloader assert fix or removal as above (this PR or a linked follow-up), and (2) a quick confirmation that the BlockingWorker fallback test still exercises the unchecked-close-failure branch after the Error-policy change, since that was the gap in F3. <!-- streview-comment:1566 --> -- 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]
