Rangsh commented on PR #12218: URL: https://github.com/apache/seatunnel/pull/12218#issuecomment-5979568576
@DanielLeens Thank you for another thorough from-scratch review, and for tracing the `testCriticalCallTime` / `testDelay` failures to the exact cause. You were right: the null-check I added in `4de899d4e` conflated "context rolled back" with "this task has no classloader entry". **Issue 1 (High): fixed in `36d911bd6`** - `BlockingWorker.run()` and `CooperativeTaskWorker.run()` now read `taskGroupContext.getClassLoaders()` once and treat only a `null` **map** as a rolled-back context. That is exactly what `claimJarsForClassLoaderRelease()` clears. A missing per-task entry falls through to the original behavior: the task runs, and `setContextClassLoader` receives `null` as it did before this PR. - Reading the map into a local once also keeps the NPE window closed: a concurrent claim can't null it between the check and the `get(taskId)`. - `TaskGroupContext.classLoaders` is now `volatile`, so workers reliably see the rollback thread's clear. The Javadoc now documents that a `null` map, not a missing entry, is the rollback signal, so the invariant is stated at the field rather than implied far away. Local verification (JDK 11, `seatunnel-engine-server`): - On the previous head `4de899d4e` I reproduced your exact signatures: `testCriticalCallTime:392 expected: <100> but was: <1>` and `testDelay:516/517 NoSuchElementException`. - On `36d911bd6`: `TaskExecutionServiceTest` passes, **Tests run: 22, Failures: 0, Errors: 0**. This includes `testCriticalCallTime`, `testDelay`, both rollback regressions, and the two atomic-claim tests. - `./mvnw spotless:apply` and `./mvnw -DskipTests verify` on the module are clean. **Issue 2 (High): checked against a clean `dev` baseline** - `SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck`: #12353 shows this reproduces on unmodified `dev` (dev @ `6ee0c3744` on JDK 17, and dev @ `75b60fa14` plus a comment-only change on JDK 8). SEZ9 also lists completed `dev` `Build` runs (`34801745423`, `34821874806`, `34935118103`, `34995029902`, `35063682504`, `35219017572`), each with at least one red `engine-v2-it` leg, and confirmed this test as the cause on the 09-17 run. Your own classification there points at `SubPlan.addPhysicalVertexCallBack` / `getPipelineEndState()` giving `failedTaskNum > 0` priority over a requested cancel. The fix in your #12311 touches `CoordinatorService` only; neither touches `TaskExecutionService` / `TaskGroupContext`. - `BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure`: your #12313 reports it failing on `dev` itself (`34821874806`: observed 0, `34801745423`: observed 2). The root cause is in the test fixture: the 10M-row single split keeps `FakeSourceReader` re-entering the checkpoint lock, which starves `triggerBarrier`. That fix only changes the IT and its `.conf`. On whether the task-skipping bug could have caused it: as you noted, production `deployTask` always puts a classloader for every task, so that branch was not reachable in the E2E path. And as of `36d911bd6` the branch can't skip a non-rolled-back task at all. So both E2E failures predate this PR and are tracked by #12353 → #12311 and #12313. Once those land in `dev`, I'm happy to rebase so `engine-v2-it` is re-checked on a clean base. The PR is currently `MERGEABLE` against `dev`, and fork CI will re-run on the new head. @SEZ9 FYI, since you followed up on the CI item last round: the unit-test regression is fixed, and the remaining `engine-v2-it` failures map to the issues above. Please take another look when you have time. Thanks again for the careful reviews! -- 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]
