DanielLeens commented on PR #11557:
URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-5287979248
# Re-review note
There is a genuinely new commit since my last review (`4926280925`,
submitted against head `2da2feed6b68636e2c5e9bb55c58378566eb7aa6`): the current
head is now `2eb0e8378213a526a8d17b79c4c5c04084cb2aed`, one commit ahead
(`2eb0e83782`, "Merge branch 'dev' into
davidzollo_source-managed-runtime-phase0").
I did not take that at face value. I diffed
`2da2feed6b68636e2c5e9bb55c58378566eb7aa6..2eb0e8378213a526a8d17b79c4c5c04084cb2aed`
directly:
- `git log --oneline 2da2feed6..2eb0e83782` shows the merge commit plus 24
commits that are all pre-existing `dev` history (docs-only connector-doc
improvements, an unrelated NATS JetStream sink connector, CI/E2E infra changes,
a shade fix, and `[Test][API] Tolerate the pre-existing close() race in the
schema-change worker-failure test (#11726)`), none authored on this branch.
- `git diff 2da2feed6..2eb0e83782 --stat -- seatunnel-engine/
seatunnel-api/` shows exactly **one** file: a 16-line addition to
`MultiTableSinkWriterSchemaChangeBroadcastTest.java` (wrapping an existing
`close()` call in a try/catch to tolerate a pre-existing race) — and that file
is entirely unrelated to the managed-source-runtime feature (it's
`seatunnel-api`'s multi-table-sink writer test, brought in by the `dev` merge,
not touched by this PR's own commits).
- `git diff 2da2feed6..2eb0e83782 --stat -- '*kafka*' '*iceberg*' '*fake*'`
touches only E2E `.conf` fixture files and `FlinkMetricsIT.java` (all
`dev`-side, unrelated connectors like BigQuery/Druid/NATS), not the
Kafka/Iceberg/Fake managed-source pilot code.
- The full `apache/dev...review-pr-11557` diff is now **104 files,
+14901/-96** — a 3-line net delta from the `104 files, +14898/-96` I measured
on `9c2ffde8` two rounds ago, entirely accounted for by the merge above.
Conclusion: this commit is a pure `dev`-sync merge. Zero lines of this PR's
own managed-source-runtime code (SPI, selector, `ManagedSource*Runtime`,
`SourceAssignmentTracker`, `SourceCommandCodec`, the Hazelcast admission
operations, `SourceFlowLifeCycle`, `SourceSplitEnumeratorTask`, or the
Kafka/Iceberg/Fake pilots) changed. There is no carryover apology needed and
nothing to re-derive from a blind full re-read a fourth time — but per protocol
I still independently re-verified the three highest-severity findings against
the *current* head's actual source, from scratch, without trusting my own prior
line-number claims:
- `SourceSplitEnumeratorTask.java` `stateProcess()`: the `RUNNING` case
still calls `managedCoordinatorRuntime.runOneTurn()` when on the managed lane,
and so does `WAITING_RESTORE`; the `PREPARE_CLOSE` case is still exactly `if
(closeCalled) { currState = CLOSED; } else { Thread.sleep(100); }` with no
`runOneTurn()` call anywhere in that branch. A managed bounded job's final
checkpoint barrier is still admitted-but-never-drained during `PREPARE_CLOSE`,
so `notifyCheckpointComplete()` never fires, `closeCalled` never gets set, and
the job spins to a checkpoint timeout. **Issue 1 reproduces byte-for-byte at
the current head.**
- `ManagedSourceCoordinatorRuntime.restoreRuntimeState(byte[])`: the Javadoc
directly above the method still states, verbatim, that it "Deliberately does
not call `checkOwner()` ... the caller's restore-operation thread is never the
task's own execution thread, and asserting ownership here would poison it
before the first legitimate owner-thread call" — and the method body still
immediately calls `assignmentTracker.restore(...)`, whose first statement in
`SourceAssignmentTracker` is `checkOwner()`, lazily binding `ownerThreadId` to
the transient restore-operation pool thread. Every subsequent owner-loop call
(`markRestoredSplitsIncluded`, `applyBarrier`, etc.) then throws "accessed
outside its coordinator event loop." **Issue 2 reproduces byte-for-byte at the
current head, including the self-contradicting Javadoc.**
- `FakeSourceSplit.java`: still `@Data` over `tableId`, `splitId`, `rowNum`,
`totalRowNum`, `serialVersionUID` unchanged at `-3321891887156360959L`. This is
still a genuine, ungated identity (`equals`/`hashCode`) change reaching the
**legacy** lane regardless of whether managed mode is enabled. **Issue 6
reproduces byte-for-byte at the current head.**
I did not re-derive Issue 3 (the failover-vs-in-flight-discovery race at
`ManagedSourceCoordinatorRuntime.java` around `requireReadyReader`) from source
this round since neither `SeaTunnelSplitEnumeratorContext.java` nor
`ManagedSourceCoordinatorRuntime.java` appears anywhere in the
`2da2feed6..2eb0e83782` diff; it stands unchanged for the same reason the whole
file is untouched.
## What is new this round: CI signal, partially
`gh pr view` shows `mergeStateStatus: BLOCKED`, `reviewDecision:
REVIEW_REQUIRED`, `mergeable: MERGEABLE` — consistent with a Draft-style PR
awaiting a write-capable maintainer, not a new problem. The apache-side "Build"
check is still `QUEUED` (it is a pointer, per this repo's usual pattern). The
real signal is on the fork: `DanielLeens/seatunnel` run `31704069417` for head
`2eb0e83782` was actually `in_progress` at the time of this review (I did not
wait for it, I inspected the jobs that had already completed):
- `unit-test (11, windows-latest)` **failed** — but the actual failure is
`OptionRulesApiTest.before:73 » IllegalState Node failed to start!`, a
pre-existing Hazelcast-node-startup test that is not part of this PR's diff at
all (it does not appear in `apache/dev...review-pr-11557`'s file list) — a
known-flaky-infra failure, not a regression from this branch.
- In that same lane, every managed-source-runtime unit test that did get to
run **passed**: `ManagedCoordinatorSchedulerTest`,
`ManagedSourceCheckpointStateTest`, `ManagedSourceRuntimeMetricsTest`,
`ReaderCommandMailboxTest`, `SourceAssignmentTrackerTest`,
`SourceCommandCodecTest`, `SourceSplitEnumeratorTaskTest`,
`JobCheckpointUtilsTest` all show `Failures: 0, Errors: 0`.
- However, because of that one unrelated failure, the matrix's other
`unit-test` legs (`11 ubuntu-latest`, `8 windows-latest`, `8 ubuntu-latest`)
and — critically — **both `engine-v2-it` legs (JDK 8 and 11) were cancelled**,
exactly as happened on the previous head's run (`31451018144`, `cancelled`).
`engine-v2-it` is the lane most likely to actually exercise a managed job's
shutdown/restore transitions end-to-end (the paths in Issues 1 and 2), so this
run still does not produce the evidence that would refute those findings; it
only confirms the unit-level protocol/codec/metrics tests are healthy, which
was never in question.
No new review or comment from any maintainer other than Daniel appeared
since the last round (`gh api .../pulls/11557/reviews` and
`.../issues/11557/comments` show nothing after `nzw921rx`'s single inline
comment on `docs/zh/engines/zeta/managed-source-runtime.md:31`, already
addressed in the `4880738140` round and unaffected by this merge).
## Conclusion: Ready to merge after fixes (unchanged)
Quoting my own prior verdict, which stands: "a well-shaped,
disabled-by-default second Source execution lane whose steady-state protocol is
careful, but whose shutdown and restore paths are demonstrably broken by
source-level inspection, and CI has still never run the one test that would
prove otherwise." That remains true today — the shutdown/restore breakage
(Issues 1, 2) and the failover race (Issue 3) are still each independently
merge-blocking by direct source inspection, and the newest CI run, while
healthier than before at the unit-test layer, still could not complete the one
IT lane that would settle the question either way.
1. **Blockers — must be fixed** (unchanged, all independently re-verified
this round at the current head):
- Issue 1 — `SourceSplitEnumeratorTask.java`, `PREPARE_CLOSE` case in
`stateProcess()`: must drain/apply the queued checkpoint barrier (call
`managedCoordinatorRuntime.runOneTurn()`, or equivalent) before or while
waiting on `closeCalled`, or a bounded managed job can never complete its final
checkpoint.
- Issue 2 — `ManagedSourceCoordinatorRuntime.restoreRuntimeState()` /
`SourceAssignmentTracker.restore()`: the two methods' ownership-thread
assumptions contradict each other; either make `restore()` skip the owner check
for the restore call path, or rebind ownership explicitly after the
restore-operation thread hands off to the real owner thread.
- Issue 3 — `ManagedSourceCoordinatorRuntime.java` (`requireReadyReader`
call site near line 1480) /
`SeaTunnelSplitEnumeratorContext.registeredReaders()`: a discovery result
racing a not-yet-handshaked replacement attempt must defer rather than fail the
whole source.
2. **Recommended fixes — non-blocking**: Issues 4-5, 7-30 from the prior
full review round (`4896649794`) stand unchanged, since none of the files they
cite are touched by this merge; I am not re-listing all thirty here to avoid a
fourth copy-paste of an unchanged essay, but a write-capable maintainer picking
this PR up should read that round's full table before approving.
This PR is not ready to merge as-is. Since I am the author, this comment
cannot carry a formal review-state change on GitHub — the real conclusion is
stated here in words: **not ready to merge**, pending the fixes above and a
completed (not cancelled) `engine-v2-it` run.
--
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]
