DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5812700965
@SEZ9 — thanks for pushing back rather than taking my last comment at face value; two corrections are fair, so let me be precise this time. **Aggregate review/merge state (re-confirmed just now via the API, not the sidebar):** `reviewDecision` = `APPROVED`, `mergeable` = `MERGEABLE`, `mergeStateStatus` = `BLOCKED`. The only failing required check is `Build`. So nothing about review state (mine, yours, or anyone else's) is gating this merge right now — it's purely the `Build` check. **F4 — my mistake, it was dropped from the list, not resolved.** On the current head, `finishExecution()` (`TaskExecutionService.java:1685-1730`) already does exactly what F4 asked for: it unconditionally calls `cancelAsyncFunctionFutures(context)` (`:1720`) and `cancelTimerFlushFutures(context)` (`:1725`) for the tracker that's finishing, regardless of whether `activeGeneration` is true — i.e. a stale generation's own async functions and timer-flush tasks are cancelled too. But this method is untouched by this PR: it's part of `dev`'s existing design from #12238 (already at the merge-base), not something this PR's 3-hunk diff changes. So F4 is satisfied on `dev`, out of scope for this PR — same bucket as F1/F3/F5/F6/F8, I just missed naming it explicitly. **F2 and F7 aren't still open** — I want to be direct about this since "still pending verification" suggests otherwise. Both were confirmed with line numbers in my full review on `06c2ef03e` (https://github.com/apache/seatunnel/pull/11727#pullrequestreview-5254567038, "Response to the open @SEZ9 items" section), and I independently re-diffed and re-confirmed them again yesterday on `a80834ebf` (both files are byte-identical to `06c2ef03e`, so the same evidence applies unchanged): - F2: `BlockingWorker.run()` reads the class loader from `taskGroupExecutionTracker.context.getClassLoaders()` (`:1294` in that review's line numbering), not the shared `executionContexts` map. `context` is `private final`, assigned once in the constructor from the same object `deployLocalTask()` builds — a newer generation's context can't leak in. - F7: the only reflective access is `executionContextsOf()` in `TaskDeployStaleContextRaceTest.java` (now with a class-level Javadoc stating what it does and doesn't cover), plus the same Javadoc scoping the redeploy-vs-taskDone race to `deployLocalTask()`/tracker teardown — which is #12238/dev territory, not this diff. **F1/F3/F5/F6/F8 — direct answer to "introduced by this PR or only present on dev":** only present on `dev`, not introduced or touched by this PR. Evidence: `git grep -n "finishOwnedResources\|cancelOwnedAsyncFunctionsInPlace\|ownedContext" <head>` returns zero hits — those identifiers don't exist anywhere in the current tree, PR or dev; they were from the earlier "ownership model" commits that were stripped out on 09-13 per the discussion above. And the file-scoped diff (`git diff <merge-base> <head> -- TaskExecutionService.java`) is exactly: one `import java.io.IOException;` removal and two hunks inside `BlockingWorker.run()` (`:1276-1356`) — nothing in `deployLocalTask()`, `finishExecution()`, or `cancelAllTask()` is touched. So F1 (redeploy-vs-taskDone race on `cancellationFutures`), F3, F5, F6 and F8 all describe `dev`/#12238 code this PR doesn't reach. **CI on `a80834ebf` — correcting my own 09-23 comment, which named the wrong test.** I pulled the actual fork job logs rather than go from memory again: - `engine-v2-it (8, ubuntu-latest)` and `engine-v2-it (11, ubuntu-latest)`: **both** fail on `BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure` — not `SplitClusterFaultToleranceIT` as I said on 09-23, that was wrong and I apologize for the confusion it may have caused. This is the same known `dev` flake I flagged in my `06c2ef03e` review (fixes #12316/#12313 still open upstream). - `engine-v2-it (11, ubuntu-latest)` additionally fails `CheckpointCoordinatorFailoverIT.testStreamJobFailsAfterCheckpointTriggerDispatchFailure` — an assertion mismatch during simulated master failover (`can't find task group address from taskGroupLocation` inside `CheckpointCoordinator.notifyCheckpointCompleted`), unrelated to `BlockingWorker`/`TaskExecutionService` and not something I'd seen on earlier heads of this PR. - `all-connectors-it-2 (8 and 11, ubuntu-latest)`: `OpengaussCDCIT.testAddFieldWithRestore` — Testcontainers/Awaitility timeout plus a "container is not running" Docker error mid-test. - `all-connectors-it-7 (11, ubuntu-latest)`: `PostgresCDCIT.testPostgresCdcSnapshotOnlyAndCommittedOffsetStartupModes` — `awaitility` timeout after a replication-connection reset. - `kudu-connector-it (8 and 11, ubuntu-latest)`: cancelled, not investigated. None of these touch `seatunnel-engine-server`'s `TaskExecutionService`/`BlockingWorker` path, so none implicate this diff. 72 of 79 non-skipped jobs pass, including all four `unit-test` legs. I've done a complete, from-scratch re-review of the current head and I'm posting it as a new formal review right after this, since a new commit (the dev-merge to `a80834ebf`) landed since my last one. -- 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]
