DanielLeens commented on PR #11557:
URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-5379334489
# What Problem Does This PR Solve?
- User pain point: in Zeta today, Source concurrency ownership is fragmented
across every connector — the collector loop calls `SourceReader#pollNext` under
a connector-owned checkpoint lock, and split assignment / reader registration
travels through Hazelcast operation threads with ad-hoc synchronization that
each connector author has to get right on their own.
- Fix approach: move Reader and Split Enumerator concurrency ownership into
two engine-owned event loops (a Reader runtime and a Coordinator runtime)
behind a disabled-by-default, exact-allowlist "managed" lane, while the
existing legacy lane and the public `SourceReader`/`SplitEnumerator` SPI stay
completely untouched for every connector that has not opted in.
- One-sentence summary: this is STIP-31 phase 0, an explicitly gated Draft
with its own five-item graduation checklist (still 0/5 at this head); the
design is sound and its compatibility posture for non-adopting connectors is
genuinely strong, but at the current head it is not mergeable — it carries a
real, unresolved git conflict against `dev`, and a concurrency stall I
re-traced end-to-end (queue → admit → drain) is confirmed still present.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Housekeeping first, honestly.** This is my tenth pass over this PR (nine
prior self-reviews plus one comment from @nzw921rx). The head (`e62beaaadf98`)
has **not moved since 2026-08-14**, and I confirmed live against GitHub just
now that there is no new commit, no new review, and no new comment/reply of any
kind since my own last review at 2026-08-21T14:12:53Z — `gh pr view`'s
`updatedAt` timestamp is literally just that review comment, not a code push.
So this round is not a from-scratch re-derivation of the standing 45-issue
backlog (16 High / 3 Med-High / 17 Medium / 9 Low from the 2026-08-16 round) —
nothing has happened that would invalidate or require re-deriving it. What I
did instead was independently re-verify, from first principles in the actual
source at this exact head (not by trusting my own prior write-ups), the two
most consequential blockers, and I traced one of them one layer deeper than any
previous round did.
**Re-verified #1: the `dev` conflict is real, not a stale GitHub flag.**
`git merge-tree $(git merge-base dev refs/pr-review/11557)
refs/pr-review/11557 dev`, run fresh against the current mirror, produces an
actual `<<<<<<<`/`>>>>>>>` conflict body (not just an auto-resolved "changed in
both") in:
`seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/execution/TaskExecutionContext.java`
```java
+<<<<<<< .our
/** Returns the immutable engine deployment identity for this task
attempt. */
public long getExecutionId() {
return executionId;
+=======
+ public ClassLoaderService getClassLoaderService() {
+ return taskExecutionService.getClassLoaderService();
+>>>>>>> .their
}
```
This PR's `getExecutionId()` accessor and a `getClassLoaderService()`
accessor that has independently landed on `dev` since insert at the same
location, and neither side closes its own brace — a textbook adjacent-insertion
conflict. `gh pr view` confirms `mergeStateStatus: DIRTY`, `mergeable:
CONFLICTING` live, matching this. Not mergeable as-is regardless of code-review
outcome.
**Re-verified #2: the managed coordinator's `PREPARE_CLOSE` stall — traced
the full admit→queue→drain chain, not just the symptom.**
Previous rounds identified that `runOneTurn()` stops being called once the
state machine leaves `RUNNING`. I went one step further this round and traced
*why* that's fatal rather than just harmless idling. In
`SourceSplitEnumeratorTask.java`:
```java
case RUNNING:
...
} else if (isManagedCoordinatorRuntime()) {
if (!managedCoordinatorRuntime.runOneTurn()) {
Thread.sleep(managedSourceRuntimeConfig.getIdleWaitMillis());
}
}
break;
case PREPARE_CLOSE:
if (closeCalled) {
currState = CLOSED;
} else {
Thread.sleep(100);
}
break;
```
`runOneTurn()` is called from exactly two places in the whole codebase —
this `RUNNING` branch, and a bootstrap/restore call — never from
`PREPARE_CLOSE`. The question is whether anything else can set `closeCalled =
true` for the managed lane without `runOneTurn()`. I traced it:
`notifyCheckpointComplete`/`notifyCheckpointAborted` (the only two call sites
in this class that ever invoke `closeCall()` on the legacy path) **both return
early for the managed lane** before reaching their own `closeCall()` line,
delegating instead to
`managedCoordinatorRuntime.admitCheckpointComplete(checkpointId)` /
`admitCheckpointAborted(...)`. Those two methods, in
`ManagedSourceCoordinatorRuntime.java`, do **not** execute the callback — they
only call `requireControlAdmission(() -> applyCheckpointComplete(checkpointId),
...)`, which enqueues a `ControlEvent`.
`applyCheckpointComplete`/`applyCheckpointAborted` — the only methods that ever
call `task.managedCheckpointCallbackFinished(checkpoi
ntId)`, which is the *only* path that sets `closeCalled = true` for the
managed lane — are private methods with zero direct callers; they are only
reachable by whatever dequeues and executes `ControlEvent`s, which per
`runOneTurn()`'s own doc comment ("Executes one connector callback, scheduler
result, or admitted Reader command") is exactly what `runOneTurn()` does.
So the full chain is: checkpoint-complete notification arrives → gets
enqueued → is only ever drained by `runOneTurn()` → `runOneTurn()` is only ever
called from `RUNNING` → state has already moved to `PREPARE_CLOSE` → the event
that would flip `closeCalled` to `true` is queued forever → `PREPARE_CLOSE`
polls `closeCalled` every 100ms, forever. This is not a theoretical race
window; it is a real deadlock in the managed lane's own bounded-job /
savepoint-driven close sequence, confirmed by direct call-graph tracing at the
current head, independent of and more precise than my own prior write-up of the
same issue.
**Runtime path (managed lane only — bypassed entirely for every connector
not on the exact allowlist):**
```text
Zeta job submission -> PhysicalPlanGenerator selects legacy vs managed lane
(re-derived live on
every JobMaster#init, including restore)
-> managed lane: SourceSplitEnumeratorTask drives
ManagedSourceCoordinatorRuntime.runOneTurn()
only while state == RUNNING
-> checkpoint-complete/aborted notifications admit a ControlEvent into the
coordinator's queue,
drained only by runOneTurn() -- never drained once PREPARE_CLOSE is
entered (deadlock, above)
-> ManagedSourceReaderRuntime: bounded mailbox, admission control,
checkpoint-aware split ownership
-> checkpoint: explicit Reader/coordinator checkpoint serializers with
capability digest and
connector state version
```
Because the feature is fail-closed and disabled by default (empty allowlist
+ global kill switch), the normal execution path for every currently-shipping
connector never reaches any of this — this bounds the blast radius to "not a
production risk today" while still being a real correctness gap in code that
intends to graduate out of Draft.
**Responding to @nzw921rx's design comment (Step 0.5, unresolved thread,
2026-08-06 — no reply from me on record):** you raised whether an exposed
feature switch is the right long-term shape, versus connectors simply failing
fast if they don't support a configured feature, worrying an exposed switch
could let users silently weaken semantics. Looking at the actual mechanism
again: the switch here is an *exact allowlist* plus a *global kill switch*,
both defaulting to empty/off — it is not a per-job opt-out that a user can flip
to silently degrade an already-adopted connector's guarantees; it gates whether
a connector's engine-side runtime has been migrated at all. I think that
structurally addresses your concern: it's closer to a migration flag than a
semantics-weakening user knob, and the code already supports your preferred end
state — as connectors are added to the allowlist over time, the switch's
practical effect shrinks toward zero, exactly as you describe. I'd frame it
as "keep the switch, but the goal is to make it irrelevant by adoption, not
to remove it up front."
## 1.2 Compatibility Impact
**Fully compatible in its current, disabled-by-default state.**
`SourceReader.pollNext(...)`, `Collector.getCheckpointLock()`, and existing
`SplitEnumerator` methods remain source- and binary-compatible; no connector is
forced onto the managed lane; the kill switch and exact allowlist mean the
legacy lane's behavior for every existing connector is byte-for-byte unchanged.
Re-verified this claim directly rather than taking it on trust — this is the
one property of the PR I'd call unconditionally strong.
## 1.3 Performance / Side-Effect Analysis
Not independently re-benchmarked this round — no soak/performance evidence
is attached to the PR, which the PR's own graduation checklist requires before
Draft exit. The bounded-queue, reserved-control-capacity,
reused-execution-thread design is architecturally sound on paper but still
unproven without that evidence.
## 1.4 Error Handling and Logging
**Issue A (High) — Unresolved git conflict against `dev` blocks a clean
rebase/merge**
- **Location**:
`seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/execution/TaskExecutionContext.java`
(this PR's `getExecutionId()` vs. `dev`'s `getClassLoaderService()`).
- **Problem**: Confirmed via a fresh `git merge-tree` against current `dev`
this round — a real, unresolved textual conflict, matching GitHub's live
`mergeable: CONFLICTING` / `mergeStateStatus: DIRTY`.
- **Potential risk**: The PR cannot be merged as-is; a naive merge commit
(rather than a deliberate rebase) risks silently dropping one side's accessor.
- **Best improvement**: Rebase onto current `dev`, keep both
`getExecutionId()` and `getClassLoaderService()`.
- **Severity**: High.
- **Raised by another reviewer**: No — carryover from my own 08-21 round,
independently re-confirmed this round with fresh tooling output.
**Issue B (High) — Managed coordinator never drains queued
checkpoint-complete/aborted control events once `PREPARE_CLOSE` is entered**
- **Location**: `SourceSplitEnumeratorTask.java` (`RUNNING` branch calls
`runOneTurn()`; `PREPARE_CLOSE` branch only polls `closeCalled`);
`ManagedSourceCoordinatorRuntime.java`
(`admitCheckpointComplete`/`admitCheckpointAborted` only enqueue via
`requireControlAdmission`; `applyCheckpointComplete`/`applyCheckpointAborted` —
the only path to `task.managedCheckpointCallbackFinished(...)` → `closeCall()`
— are unreachable except by `runOneTurn()`'s internal drain).
- **Problem**: Traced the complete admit→queue→drain chain this round
(deeper than the prior write-up, which asserted the symptom without tracing why
`closeCalled` can never flip). Confirmed at the current, unchanged head.
- **Potential risk**: Real deadlock — a managed-lane job whose close
sequence depends on a checkpoint-complete/aborted callback (bounded job
completion, savepoint-driven close) can stall indefinitely in `PREPARE_CLOSE`,
sleeping 100ms forever.
- **Best improvement**: Keep draining the coordinator's control-event queue
while in `PREPARE_CLOSE` (either keep calling `runOneTurn()` or add an explicit
drain-remaining-control-events step) until the coordinator itself reports
nothing left to process.
- **Severity**: High.
- **Raised by another reviewer**: No — carryover from my own prior round;
independently re-verified and the causal chain traced one layer deeper this
round.
**Issue C (High, self-declared blocker) — PR's own Draft graduation
checklist is 0/5 satisfied**
- **Location**: PR description, "Draft graduation gates" section.
- **Problem**: All five gates (clean CI at current head, deterministic
failover/fault-injection campaigns, managed-connector conformance coverage,
Kafka/Iceberg/blocking-control/high-split-count performance results, maintainer
review) remain unchecked, consistent with this round's findings (CI not clean,
Issue B open, real git conflict).
- **Potential risk**: None beyond what's already disclosed by the author.
- **Best improvement**: N/A — this is the PR's own stated bar, not yet met.
- **Severity**: High (self-declared).
No independent re-audit of logging coverage across the ~1,920-line
coordinator runtime this round (standing Issue 16 from the 08-16 round:
effectively zero logging, multiple managed operations collapse distinct failure
causes to a generic outcome) — no code has changed since that finding, so I
have no basis to believe it's changed either.
# 2. Code Quality Assessment
## 2.1 Coding Standards
No independent re-audit across all 95 files this round given the file count
and the depth of the nine prior passes; no commit has landed since the last
audit that would change the answer.
## 2.2 Test Coverage and Test Stability
**Mandatory stability rating: High risk.** Two independent, still-current
reasons: (1) the standing review's finding that no test covers any of the four
legacy↔managed restore paths — the highest-risk compatibility surface in the PR
— remains unaddressed; (2) I re-checked CI live at this exact head: fork run
`31787886417` (`DanielLeens/seatunnel`) still shows `unit-test (11,
windows-latest)` as `failure` (`RestApiSubmitJobStartWithSavePointTest`,
`IllegalStateException: Node failed to start!`), with `engine-v2-it` and
`unit-test (11, ubuntu-latest)` `cancelled` and the integration-test-part jobs
`skipped` as a result of that gating failure. This Windows/Hazelcast-bootstrap
symptom matches a previously-documented environmental flake pattern (different
test class each occurrence, ~300s timeout, windows-latest only) rather than
something tied to this PR's code, but the run has not been re-triggered since
08-14 and CI is not currently green at this head either way.
## 2.3 Documentation Updates
No independent re-verification this round. Standing finding:
`docs/en/introduction/concepts/incompatible-changes.md` is not updated for this
feature (arguably correctly, since the feature is fully backward compatible
while disabled — but worth an explicit note once it graduates), and the
managed-source-runtime doc's rollback section wording was flagged as ambiguous
against the surrounding failover-stability claims; unchanged since flagged.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution — Long-term solution (design) / Temporary
workaround (current implementation maturity)
The two-tier design — an exact, versioned, opt-in capability allowlist with
a global kill switch, legacy lane fully untouched — is the right shape for a
change this invasive to Zeta's Source execution model, and directly addresses
@nzw921rx's design concern as discussed in 1.1: it's a migration gate, not a
semantics-weakening user knob. But "0/5 graduation gates, and a real deadlock I
just re-traced end-to-end" means the *implementation* has not yet earned
"long-term solution" independent of the design being right.
## 3.2 Maintainability
The near-absence of logging in the ~1,920-line coordinator runtime (standing
finding) is a real maintainability risk specifically because this code exists
to take over concurrency reasoning that used to live, visibly, in connector
implementations. If Issue B's deadlock fires in production, there would
currently be nothing in the logs pointing at it.
## 3.3 Extensibility
The `ManagedSourceReader` bridge and the Fake/Kafka/Iceberg pilots
demonstrate a workable extension pattern; Iceberg's choice to adopt only the
coordinator runtime while keeping its Reader on the legacy lane is a sensible
incremental-adoption example.
## 3.4 Historical-Version Compatibility
Fully compatible in the feature's current disabled-by-default state, as in
1.2. Standing finding, not independently re-verified this round: `SourceConfig`
gains five new serialized fields with no `serialVersionUID`, a rolling-upgrade
risk specifically for the master→worker `TaskGroup` serialization path once/if
the managed lane is enabled for any connector.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| A | Unresolved git conflict against current `dev` blocks merge —
reconfirmed fresh this round | `TaskExecutionContext.java` | High |
| B | Managed coordinator never drains queued checkpoint-complete/aborted
events once `PREPARE_CLOSE` is entered — full admit→queue→drain chain traced
this round | `SourceSplitEnumeratorTask.java`,
`ManagedSourceCoordinatorRuntime.java` | High |
| C | PR's own Draft graduation checklist is 0/5 satisfied | PR description
| High (self-declared) |
| — | Standing backlog from the 2026-08-16 full-review round, unchanged
head, still authoritative: 45 numbered issues (16 High / 3 Med-High / 17 Medium
/ 9 Low), spanning coordinator restore-thread binding, lane re-derivation on
every `JobMaster#init`, unbounded checkpoint-proof growth, unimplemented
coordinator epoch-rebase, and near-zero coordinator logging | PR review thread,
round dated 2026-08-16 | 16 High and below |
| — | CI: `unit-test (11, windows-latest)` failure at current head, likely
the documented Windows/Hazelcast bootstrap flake but not re-run to a
confirmed-clean state | fork run 31787886417 | Needs re-run to confirm |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
**1. Blockers — must be fixed**
1. **Issue A (High)** — Rebase onto current `dev` and resolve the real
conflict in `TaskExecutionContext.java`.
2. **Issue B (High)** — Fix the `PREPARE_CLOSE` control-event drain gap;
I've now traced the exact mechanism (admit→queue→drain) so this should be a
well-scoped fix rather than a search.
3. **Issue C / standing backlog** — Work through the standing 16
High-severity findings from the 2026-08-16 round systematically, in particular
the fault-injection/failover campaigns and managed-connector conformance
coverage the PR's own checklist calls for, before this leaves Draft.
4. Get a fresh, fully green CI run at whatever head results from the above,
and confirm the Windows unit-test failure doesn't reproduce (or independently
confirm it's the known environmental flake).
**2. Recommended fixes — non-blocking**
Follow the standing review's own Medium/Low prioritization from the
2026-08-16 round once the High items above are closed.
**Overall assessment.** Nothing has changed at the code level since my last
review three days ago, and I want to be upfront that most of this round's value
is two things: independently reconfirming, with fresh tooling and a deeper
call-chain trace, that the two most consequential blockers are real and
unchanged — plus responding to @nzw921rx's open design question, which had gone
unanswered. The direction of STIP-31 phase 0 is right, and its compatibility
story for every connector not opting in is genuinely solid — I keep re-checking
that claim rather than taking my own past word for it, and it keeps holding up.
But this is still, correctly, a Draft: it has an actual unresolved conflict
against `dev`, a concurrency deadlock I can now trace start to finish, and a
graduation checklist the author's own PR description says is 0/5 done. As the
author, my view here isn't an independent approval — a write-capable maintainer
still needs to do the final pass, and per the PR's own sta
ted gates, that should wait until the fault-injection, conformance, and
performance evidence the description calls for is actually attached.
--
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]
