DanielLeens commented on PR #12391:
URL: https://github.com/apache/seatunnel/pull/12391#issuecomment-6310075033
Thanks @SEZ9 for going through the current head. I re-read the diff at
`2a5e21fe` for each of your questions, so here is what the code shows (this is
a merge-only head, the PR content is the same as `6ad74f1f`).
**2. Does the BlockingWorker fallback rethrow an `Error`?** It does not
downgrade it to a log line. In `TaskExecutionService.BlockingWorker.run()` the
fallback is now `try { tracker.task.close(); } catch (Exception e) {
logger.severe("Close task error", e); }` inside an outer `try { ... } finally {
setContextClassLoader(oldClassLoader); }`. An `Error` thrown by `close()` is
not an `Exception`, so it skips the inner catch, the outer `finally` restores
the classloader, and the `Error` then propagates out of `run()` (to the
thread's uncaught-exception handling). Only `Exception` subtypes are logged and
absorbed. One nuance for the record: because `taskDone(t)` has already been
called at that point, such an `Error` is not recorded on the execution tracker,
which matches the pre-PR behaviour for the fallback path.
**3a. `closeSuper()` throwing while the lifecycle loop still runs.**
`SeaTunnelTaskStateTest#testClosePreservesSuperCloseFailureAndAttachesLifecycleFailuresAsSuppressed`
stubs the parent close to fail, then asserts that the parent failure is the
primary one and the lifecycle failures are attached as suppressed.
`testCloseSkipsLifecycleLoopWhenAllCyclesNull` covers the null case.
**3b. Fallback hitting the already-closed latch.** Here I agree with the
gap, and it is the same point as Issue 1 in my last review, which is still
open. The latch itself is covered at the `SeaTunnelTask` level by
`testCloseIsIdempotentAndClosesEachLifecycleOnlyOnce`. But
`TaskExecutionServiceTest#testBlockingWorkerFallbackCloseWhenCallAndCloseThrowRuntimeException`
uses `CallAndCloseThrowRuntimeExceptionTask`, which is not a `SeaTunnelTask`
and has no latch. The test also asserts only `FAILED` and `closeCalled`, which
would still hold if the fallback catch went back to `catch (IOException)` (the
`RuntimeException` would escape `run()` after the future has already
completed). So it does not currently guard either production edit in
`BlockingWorker`. @SeaSand1024, the concrete way to make it discriminate is in
my last review: have the test task's `close()` throw an `AssertionError`,
record the worker thread, and assert after completion that its context
classloader was restored.
**4. Javadoc.** The `close()` Javadoc matches the flow: `@throws
IOException` when the primary failure is an `IOException`, unchecked exceptions
and `Error`s rethrown unwrapped via `sneakyThrow`, later failures attached as
suppressed, `Error` preferred over `Exception`, early stop only for
`VirtualMachineError` and `ThreadDeath`. `closeSuper()` has no `@throws
IOException` tag even though it declares it, which is a tiny nit.
**5. `allCycles` null guard.** Yes: `close()` now wraps the loop in `if
(allCycles != null)`, so a failure in `init()` before `allCycles` is assigned
no longer NPEs in the fallback close. It is exercised by
`testCloseSkipsLifecycleLoopWhenAllCyclesNull`.
**CI.** Unchanged: the apache-side `Build` for `2a5e21fe` is
`action_required` and the fork has no run for this SHA, so this head has not
been tested. A fork `Build` run (or a fresh push) is still needed, after which
re-running only failed jobs should suffice.
My remaining open items from the last review are Issue 1 (Medium, the
BlockingWorker test) and the two Low notes (a `VirtualMachineError` ending up
only as suppressed, and `closed` being null for an old-format serialized task).
None of them are production correctness problems in the common path.
--
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]
