DanielLeens commented on PR #11557: URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-5340801018
# What Problem Does This PR Solve? **One sentence (unchanged from the last round):** this PR implements STIP-31 by moving Source callback serialization, split-ownership tracking, command admission and checkpoint ordering out of each connector and into two engine-owned event loops, behind a disabled-by-default, per-connector allowlisted "managed lane" that runs alongside the untouched legacy lane. # Re-verification note (why this comment is short) This is my seventh self-review round on this PR. The last complete review I posted (2026-08-16, review id `4945122742`) was already evaluated against the **exact current head**, `e62beaaadf98132b7058f6f39de83238f4cfcb5e`. I re-checked that fact rather than assuming it: I fetched the head into a clean worktree and diffed it against `2eb0e8378213a526a8d17b79c4c5c04084cb2aed` (the head that review covered) — the diff touches only `couchbase`/`kafka`/`prometheus`/CDC/SQL-transform files pulled in by a `dev` sync merge; **zero managed-source-runtime files changed** (`git diff --name-only <old>..<new> | grep -iE "managed|SourceFlowLifeCycle|SourceSplitEnumeratorTask|SourceAssignmentTracker|SourceCommandCodec"` returns nothing). Per this project's own review rule, a version that has not received a new commit does not get a second complete re-enumeration of its issue list — that would just be noise. So instead of re-posting the full 45-issue report, I independently re-derived (not re-asserted) the three highest-impact findings straight from the current source, and I'm reporting two facts that **are** new since 2026-08-16: the merge-conflict state and the CI rerun outcome. ## Independent re-verification of the most severe findings, against current source - **Issue 1 (High) — confirmed, byte-for-byte.** `SourceSplitEnumeratorTask.stateProcess()`, `PREPARE_CLOSE` branch (`SourceSplitEnumeratorTask.java:466-472`) only checks `closeCalled` and sleeps 100ms — it never calls `managedCoordinatorRuntime.runOneTurn()`, unlike the `RUNNING` branch three cases above it (`:458-461`). `notifyCheckpointComplete` on the managed path (`:522-527`) only enqueues (`admitCheckpointComplete`) and returns; `closeCall()` is reached only via `managedCheckpointCallbackFinished` (`:622-624`), which only runs if a turn drains that event. Once the state machine is in `PREPARE_CLOSE`, nothing drains it. **This is a real, currently-reachable hang: a bounded managed job (and any managed savepoint) parks its enumerator task thread in `Thread.sleep(100)` forever after the reader finishes**, holding a task slot and, per Issue 13 (unbounded `checkpointAssignmentProofs`/`uncheckpointedAssignmentSplitIds`, pruned only on checkpoint completion), continuing to grow unreclaimed state for as long as other checkpoints keep landing before the stall is noticed. I'm calling this out prominently because it combines an unbounded-growth resource leak with a stuck checkpoint/close path — the two conditions this project's stability-risk protocol asks me to flag explicitly. I want to be precise about the blast radius, though: the lane is disabled by default and per-connector allowlisted, so with shipped defaults this is unreachable — it is a hard blocker for shipping the *feature*, not a risk to today's default-config clusters. - **Issue 3's mechanism (High) — confirmed.** `ManagedSourceRuntimeSelector.select(SeaTunnelSource, ManagedSourceRuntimeConfig)` (`ManagedSourceRuntimeSelector.java:32-47`) takes a live `ManagedSourceRuntimeConfig`, not anything persisted about a job's prior lane. Its only caller, `PhysicalPlanGenerator.java:618`, is reached from `JobMaster.init()` → `PlanUtils.fromLogicalDAG(..., engineConfig)` (`JobMaster.java:289-301`) using the live `engineConfig` field, and `init()` is the same method used for both fresh submission and `restart=true` (restore/failover) — there is no branch that reads a persisted lane for the restart case. This reconfirms that lane selection is recomputed from cluster-global live config on every restore, which is the root of the two-way-door / mixed-fleet compatibility trap described in the 2026-08-16 review. I did not re-run the full trace on the remaining 43 issues; they were exhaustively re-derived from scratch in the 2026-08-16 round specifically because that round also found a zero-diff (that review states it re-traced from a cold start rather than re-asserting prior line numbers). Spot-checking the two highest-severity, highest-blast-radius items and getting an exact match gives me high confidence the rest of that list is still accurate. If anyone reviewing this wants a specific one of the other 43 re-verified against current source, ask and I'll check it directly rather than asserting from memory. ## New fact 1 — this branch now has real merge conflicts with `dev` `gh api repos/apache/seatunnel/pulls/11557` currently reports `mergeable_state=CONFLICTING`, `mergeStateStatus=DIRTY`. `compare/dev...e62beaaadf98132b7058f6f39de83238f4cfcb5e` reports `status=diverged`, `ahead_by=18`, `behind_by=28`. Neither fact was in the 2026-08-16 report. This branch needs a rebase/sync onto current `dev` and the conflicts resolved before it can be re-evaluated for merge, independent of everything else in this thread — I'll do that as a separate follow-up push, not folded into this comment. ## New fact 2 — CI was rerun since the last review; same conclusion, different symptom The `Build` run I evaluated on 2026-08-16 (`31787886417`, at this same head) has since been rerun (now `run_attempt=3`, completed 2026-08-18). The specific failure moved: this attempt failed `unit-test (11, windows-latest)` at `RestApiSubmitJobStartWithSavePointTest.setUp:97 » IllegalState: Node failed to start!`, not the `unit-test (8, windows-latest)` failure I diagnosed last round. This is the same class of known, environment-only Hazelcast-node-bootstrap timeout on `windows-latest` that has hit a different engine-server test class on every occurrence I've seen across multiple unrelated PRs — not something this PR's diff can cause. What has **not** changed, and is the fact that actually matters here: the fail-fast from that failure cancelled `engine-v2-it` for both Java 8 and Java 11 again, so `ManagedSourceRuntimeIT` still did not execute. That is now five consecutive evaluations across this PR's history — 2026-08-13, -14, -16, and both attempts of the -18 rerun — with zero execution evidence for the managed lane's shutdown/restore/cancel paths. Job counts this attempt: 75 passed, 1 failed (the Windows flake above), 3 cancelled (fail-fast), 10 skipped. # 5. Merge Recommendation **Conclusion: Request changes — do not merge.** Unchanged from 2026-08-16: 16 High-severity findings remain open (see the full numbered list, Issues 1-45, in review `4945122742`), the compatibility conclusion is PARTIALLY INCOMPATIBLE with an unrecoverable mixed-fleet state reachable by following this PR's own documented rollout (Issue 3, re-confirmed above), and there is still zero CI evidence at any head in this PR's history that the managed lane's shutdown, restore or cancel paths execute end-to-end. Two items are additive this round, not replacements for anything above: 1. **Merge conflicts against `dev` must be resolved** (`mergeable_state=CONFLICTING`, `behind_by=28`) before this branch can be usefully re-evaluated at all. 2. Get one `engine-v2-it` run to actually complete on a head that also carries the Issue 1 fix — this remains the single piece of evidence most likely to move the assessment, and it is now overdue across five straight evaluations. Posted as a comment, not an approval or change-request review: I authored this PR and GitHub blocks self-approval, so this needs an independent maintainer regardless of my own conclusion. The PR is correctly still in Draft. -- 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]
