[Fix][Zeta] Resolve transform dependencies without hangs or dropped inputs by goutamadwant · Pull Request #12079 · apache/seatunnel

goutamadwant · 2026-09-04T03:28:57Z

Purpose of this pull request

Closes #12077.

This PR fixes transform dependency resolution in the Zeta runtime parser and connect dry-run validator.

The previous retry queue could loop indefinitely on a dependency cycle. It also considered a multi-input transform ready when only some configured input IDs were available, silently removing unresolved upstream edges from the generated DAG. Reverse-declared valid chains repeatedly retried transforms and performed plugin discovery before readiness was known.

The change:

detects transform cycles in linear time and reports a deterministic cycle path;
requires every distinct configured input ID before creating or validating a transform;
resolves dependencies with an indexed ready scheduler in O((V + E) log V) time and O(V + E) memory;
delays plugin discovery until dependency readiness is established;
preserves the legacy retry queue's evaluation order and generated action indexes for existing valid DAGs;
checks action-index arithmetic before narrowing to the legacy integer API; and
preserves the intentional single-transform and terminal explicit-empty-input fallbacks while rejecting ambiguous unresolved graphs.

No public API, configuration option, dependency, or default behavior is added.

Does this PR introduce any user-facing change?

Yes.

Previously, a cyclic transform graph could leave submission or --dry-run connect running indefinitely. The same resolver could create an out-of-order multi-input transform from only the inputs already available, silently dropping the remaining configured upstreams.

After this change, cycles and unresolved dependencies fail promptly with a contextual configuration error. Valid out-of-order DAGs wait for all inputs and retain every upstream action/schema. Correctly ordered jobs, duplicate input IDs, generated action names, result order, and the existing compatibility fallbacks remain unchanged.

How was this patch tested?

Earlier validation on both supported JDK lines (historical results, not a fresh execution of the September 21 evidence review):

Java 8 (1.8.0_172): ConfigParserUtilTest 9/9, MultipleTableJobConfigParserTest 21/21, and SeaTunnelConfValidateCommandTest 29/29 passed.
Java 11 (11.0.19): the same suites passed 9/9, 21/21, and 29/29.
Java 8 and Java 11 skip-tests verification passed for seatunnel-engine-core, seatunnel-engine-client, and seatunnel-starter, including Spotless, compilation, and packaging.
git diff --check passed.

Coverage includes two-node and self cycles, partially and wholly unresolved inputs, mixed source/transform inputs, duplicate input IDs, valid reverse-declared chains, runtime and connect dry-run paths, simple and explicit-empty fallback compatibility, terminal sink selection, and action-index boundaries.

A wider aggregate reactor attempt was not counted as passing because it stopped before the affected modules in unchanged seatunnel-config-shade generated sources with missing ConfigException/ConfigOrigin symbols. The affected three-module verification above passes on both Java versions.

Current review evidence

The runtime parser and connect dry-run use the same TransformDependencyScheduler in engine-core. Waiting lists are consumed once with waitingByInputId.remove, so duplicate output producers cannot satisfy another required input. Explicit self-cycles are rejected before legacy omitted-input fallback. The current regression suite includes duplicate unnamed outputs, terminal omitted-input schema binding, default-first binding and single explicit self-cycles.

Compatibility and migration notes: English and Chinese.

The September 21 follow-up is a source/test evidence audit, not another test run or a claim of green aggregate CI. The optional getOutputId cleanup remains a separate follow-up, not an implemented part of this PR.

Check list

No new Jar binary package or dependency is added; the New License Guide does not apply.
English and Chinese migration guidance documents the affected invalid/ambiguous dependency cases.
English and Chinese incompatible-changes.md notes are included; supported valid DAG ordering, generated names and compatibility fallbacks are preserved.
The connector checklist is not applicable; this PR changes engine parsing and connect dry-run validation only.

…nputs

DanielLeens

What Problem Does This PR Solve?

The legacy transform-dependency resolver in MultipleTableJobConfigParser#parseTransforms (and its near-duplicate in DryRunConnectValidator) used a Queue<Config> retry loop: poll a transform config, and if its declared inputs are not yet available, push it back to the tail of the queue and try the next one. Two real defects follow from that design:

If a transform's plugin_input can never be satisfied (typo, forward reference that never resolves, or a genuine dependency cycle), the queue never empties and parseTransforms/--dry-run connect loops forever (the "hangs" in the title).
A multi-input transform was considered "ready" as soon as any of its inputs matched an existing table — the remaining, still-unresolved input ids were silently dropped from the generated action's input set instead of being waited for, so the produced DAG silently missed edges.

This PR replaces the retry queue with an indexed, Kahn-style ready-scheduler (scheduleTransforms) that resolves transforms only once every configured input id is available, detects cycles/unresolvable graphs in linear/O((V+E) log V) time instead of looping, and (for the engine-core path) reproduces the exact legacy poll-count so that default action names (Transform[<index>]-<plugin>) are unchanged for every DAG that previously resolved correctly.

1. Code Change Review

1.1 Core Logic Analysis

Before (MultipleTableJobConfigParser, dev HEAD):

Queue<Config> configList = new LinkedList<>(transformConfigs);
int index = 0;
while (!configList.isEmpty()) {
    parseTransform(index++, configList, classLoader, tableWithActionMap, usedTransformNames);
}
...
List<Tuple2<CatalogTable, Action>> inputs =
    inputIds.stream().map(tableWithActionMap::get).filter(Objects::nonNull)
            .flatMap(Collection::stream).collect(Collectors.toList());
if (inputs.isEmpty()) {
    if (transforms.isEmpty()) {
        inputs = findLast(tableWithActionMap);      // silent "just chain to the last table" fallback
    } else {
        transforms.offer(config);                    // requeue forever if never satisfied
        return;
    }
}

After: scheduleTransforms() builds, for every transform, its exact set of input ids and a waitingByInputId reverse index. A TreeSet<Integer> of "ready" indexes is drained by repeatedly picking ceiling(queueHeadIndex) (wrapping to first()), which — see below — is a provably faithful re-encoding of "next item in the rotated retry queue". Every configured input id must be present in tableWithActionMap/availableOutputIds before a transform is scheduled (missingInputIds must be empty); anything left unresolved after the ready-set drains is either the legacy single-transform/empty-input fallback or a hard JobDefineCheckException.

Key Findings

Correctness of the legacy-order emulation is real, not decorative. I traced the round-robin retry queue by hand: because unready items are always moved to the tail in the order they were polled, the "remaining queue" at any point in time is exactly a rotation of the original index order starting at the position right after the last successfully-removed item. queueHeadIndex plays the role of that rotation point, and readyTransformIndexes.ceiling(queueHeadIndex)/first() reproduces "scan the rotated queue forward for the next ready item" without materializing the queue. nextTransformActionIndex + the Fenwick tree (createRemainingIndexTree/countRemainingIndexes) then count how many poll calls (ready or not) the legacy loop would have consumed to reach that item — this is exactly the value the legacy code fed into JobConfigParser.createTransformActionName(index, factoryId). I hand-verified this against testSchedulesLongReverseDependencyChainOnceInLegacyEvaluationOrder (512-node fully-reversed chain): the triangular-number action indexes the test asserts (511, 1022, 1532, …) match exactly what the legacy O(n²) retry loop would have produced for that adversarial ordering.
Why this matters for production, not just cosmetics: ActionStateKey.of(Action action) (seatunnel-engine-server/.../checkpoint/ActionStateKey.java:35) derives the checkpoint state key from action.getName(), and unnamed transforms get their name from createTransformActionName(index, factoryId). If this PR had produced a different index/name for an existing valid DAG, then upgrading SeaTunnel and restarting/restoring a running job from a retained checkpoint or savepoint would silently fail to rebind operator state to the correct action (state loss, not a crash). The PR's insistence on bit-for-bit legacy index parity is the right thing to prioritize here, and it is backed by a real (if unusually elaborate) test, not just asserted in the description.

Runtime path:

MultipleTableJobConfigParser#parse()
  -> ConfigParserUtil.checkGraph(sources, transforms, sinks)      [Phase 6: checkTransformCycles - new]
  -> parseTransforms(transformConfigs, classLoader, tableWithActionMap)
       -> scheduleTransforms(transformConfigs, tableWithActionMap.keySet())   [new O((V+E) log V) scheduler]
       -> parseTransform(actionIndex, config, legacyFallback, ...)            [per resolved transform, in legacy order]

This is on the normal submission path for every job with a "complex" graph (more than one source/sink or more than one transform) — checkGraph and parseTransforms are called unconditionally from parse(), including the savepoint-restore branch (isStartWithSavePoint). It is not a boundary-only fix.

Genuine (disclosed) behavior change: for a "complex" graph with 2+ transforms, the old code's findLast() fallback could trigger for any transform that happened to be the last item remaining in the retry queue when its input never resolved — not just for genuinely single-transform jobs. That means a mis-configured multi-transform job (typo'd plugin_input) could previously submit successfully while silently chaining from the wrong upstream table. testDoesNotApplyLegacyFallbackAfterAnotherTransformWasScheduled confirms the new code now throws JobDefineCheckException in that case instead. This is the correct fix (silent misrouting of data is worse than a loud submission-time error), and the PR explicitly discloses it in "Does this PR introduce any user-facing change?" — but see Issue 1 below regarding incompatible-changes.md.
The Set<URL> jarUrls = new HashSet<>(getTransformPluginJarPaths(config)); line was moved to after the input-availability check (comment: "Plugin discovery only happens after all configured dependencies are available"). This avoids repeated/unnecessary classloader jar discovery for transforms that used to be re-polled many times; also avoids doing plugin discovery for a config that will ultimately be rejected as unresolvable.
checkTransformCycles (new ConfigParserUtil Phase 6) is a textbook Kahn's-algorithm cycle check plus a linear-time concrete-cycle-path reconstruction (findTransformCycle) for the error message. I confirmed it is only invoked for "complex" graphs (checkComplexGraph), never for isSimpleGraph (≤1 source/sink/transform) — this preserves the historical "single-transform self-reference silently tolerated" behavior verified by testPreservesSimpleGraphCompatibility, while adding real cycle rejection everywhere a cycle was actually reachable before (previously the old checkInputId/checkLinked phases could not detect a self-loop at all, since a transform's own freshly-created vertex satisfies its own input check).

1.2 Compatibility Impact

Partially incompatible, and disclosed. No config option, default, API, or serialization format is added or removed. However:

A previously-succeeding multi-transform job whose plugin_input referenced a non-existent/unresolvable id (and thus silently fell back to findLast()) will now fail fast at parse time with JobDefineCheckException/ConfigCheckException. The PR body honestly states this ("Yes" to user-facing change) but the checklist marks incompatible-changes.md as "not required." I'd treat this as a genuine, if narrow, incompatibility (see Issue 1).
Cyclic transform graphs (which previously hung indefinitely) now fail fast — this is strictly a fix, not a regression, since no valid job could ever have completed from a true cycle.
Legacy default action naming/indexing for every currently-valid DAG is preserved bit-for-bit (see 1.1), which is the critical compatibility property for checkpoint/savepoint restore across an upgrade.

1.3 Performance / Side-Effect Analysis

Old: unbounded (hangs) on cyclic/unresolvable graphs; O(n²) worst case (e.g., a fully-reverse-declared chain) on valid graphs, plus repeated jar/classloader discovery for every failed poll.
New: O((V+E) log V) (TreeSet operations) for scheduling, O(V+E) memory, linear cycle detection, and jar discovery deferred to once-per-successfully-scheduled-transform. This is a straightforward win with no new unbounded structures, no new threads, no new locks — low risk of side effects for a config-parse-time-only code path (single-threaded, not on the streaming hot path).
DryRunConnectValidator carries a near-identical scheduleTransforms/ScheduledTransform implementation without the Fenwick-tree action-index machinery — correctly recognized as unnecessary there because the dry-run tool's index is purely a display counter, not a persisted state key.

1.4 Error Handling and Logging

New exceptions (JobDefineCheckException in engine-core, ConfigCheckException in the dry-run validator) carry the offending transform's output id, its missing input ids, and the currently-available output ids — good, actionable context, consistent with the project's "exceptions must include sufficient context" rule.
nextTransformActionIndex explicitly guards the int narrowing (Integer.MAX_VALUE overflow) with a clear message rather than silently wrapping to a negative/incorrect action index — good defensive coding for a value that ultimately becomes part of a checkpoint state key.
No exceptions are swallowed; no sensitive data is logged.

2. Code Quality Assessment

2.1 Coding Standards

New public/package-private methods (scheduleTransforms, nextTransformActionIndex, the Fenwick-tree helpers, checkTransformCycles, findTransformCycle) all have explanatory comments describing why, not just what — e.g. the Fenwick-tree comment correctly explains it exists to avoid replaying every unsuccessful poll. This matches the project's comment-density expectations for non-trivial/algorithmic logic.
ScheduledTransform (both copies) is a small package-private mutable holder class; fields are undocumented individually (e.g. unresolvedInputCount, legacyFallback, scheduled) but their names are self-explanatory and the class as a whole is used in a very narrow, well-commented scope. Not a blocker.
Minor: scheduleTransforms and ScheduledTransform are duplicated near-verbatim between MultipleTableJobConfigParser (engine-core) and DryRunConnectValidator (core-starter). I understand these live in different modules with different dependency graphs (engine-core is not a dependency of seatunnel-starter), so full de-duplication may not be trivial, but this is worth a follow-up ticket — two copies of a subtle scheduling algorithm are two places for a future bug to diverge.

2.2 Test Coverage and Test Stability

Coverage is genuinely thorough for this kind of change: two-node/self cycles, partially/wholly unresolved inputs, mixed source+transform inputs, duplicate input ids, out-of-order valid chains (including the 512-node worst-case reverse chain), the legacy simple-graph and explicit-empty-input fallbacks, and the Integer.MAX_VALUE action-index boundary. All three touched test classes (ConfigParserUtilTest, MultipleTableJobConfigParserTest, SeaTunnelConfValidateCommandTest) are pure, deterministic unit tests operating on in-memory Config objects — no Thread.sleep, no Awaitility, no shared mutable static state driving timing, no container startup. Stability rating: Stable. (The one static field, DryRunTestTransformFactory.CREATED_TABLES, is properly synchronized and reset per test via resetCreatedTables().)

2.3 Documentation Updates

None made; the PR checklist argues docs aren't needed because no config/API surface changed. I agree for the cycle-detection/hang fix itself. I partially disagree for the "multi-transform unresolved input now throws instead of silently falling back" behavior change — see Issue 1.

3. Architectural Soundness

3.1 Elegance of the Solution

This is a genuine, long-term fix, not a workaround — it replaces an algorithmically unsound retry loop with a proper dependency scheduler and adds the cycle-detection phase that was structurally missing from ConfigParserUtil. The extra complexity (Fenwick tree, legacy-order emulation) is not incidental complexity; it's the necessary cost of fixing the algorithm without breaking the checkpoint-state-key compatibility that depends on the old, algorithmically-poor ordering. That is a defensible trade-off for a project with the compatibility bar this one has.

3.2 Maintainability

The two duplicated scheduleTransforms implementations are the main maintainability wart (see 2.1). Everything else is well-isolated, package-private/static, and covered by focused unit tests that would catch a future regression in the scheduling order.

3.3 Extensibility

The ScheduledTransform record-like holder and the ready-set/waiting-map structure would extend cleanly if a future change needed richer dependency metadata (e.g., per-edge type information) without touching the public parseTransforms entry point.

3.4 Historical-Version Compatibility

This is the strongest part of the PR: default action names/indexes for every previously-resolvable DAG are provably unchanged (traced by hand + backed by the 512-node adversarial test), which is exactly what checkpoint/savepoint state-key rebinding (ActionStateKey) needs across an in-place upgrade. The one behavior change that isn't fully "historical-compatible" is the multi-transform silent-fallback removal, which is an intentional bug fix rather than a compatibility regression, and is disclosed in the PR body.

4. Issue Summary

#	Issue	Location	Severity
1	Disclosed user-facing behavior change (multi-transform unresolved `plugin_input` now fails fast instead of silently falling back to `findLast()`) is not reflected in `docs/en/introduction/concepts/incompatible-changes.md`, despite the PR body itself answering "Yes" to "does this introduce a user-facing change?"	`docs/en/introduction/concepts/incompatible-changes.md` (not touched)	Medium
2	`scheduleTransforms`/`ScheduledTransform` logic is duplicated near-verbatim between `MultipleTableJobConfigParser` (engine-core) and `DryRunConnectValidator` (core-starter); a future fix to the scheduling algorithm could easily be applied to only one copy	`seatunnel-engine/seatunnel-engine-core/.../MultipleTableJobConfigParser.java`, `seatunnel-core/seatunnel-starter/.../DryRunConnectValidator.java`	Low
3	`ScheduledTransform`'s individual mutable fields (`unresolvedInputCount`, `scheduled`, `legacyFallback`) have no per-field comments; class-level intent is clear from context but a one-line comment per field would help a future reader who lands only on this class	Both `ScheduledTransform` inner classes	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — none from a correctness/compatibility standpoint. I would still like Issue 1 addressed before merge: either add a short incompatible-changes.md entry describing the "multi-transform unresolved input now fails at parse time instead of silently mis-wiring" change, or have a maintainer explicitly confirm in the PR thread that a bug-fix removing silent data mis-wiring is intentionally exempted from that doc (I'd be fine with either outcome, I just want it recorded).
Recommended fixes (non-blocking) — consider de-duplicating the two scheduleTransforms copies in a follow-up (Issue 2); a short comment per ScheduledTransform field would help readability (Issue 3).

This is a well-reasoned fix for a real production hazard (an unbounded retry loop that can hang job submission/dry-run on a cyclic or malformed transform graph) and it goes to unusual lengths — successfully, as far as I can verify by hand-tracing the algorithm and cross-checking against the adversarial 512-node test — to preserve the exact legacy action-naming/indexing that checkpoint and savepoint state restoration depends on. The test suite is deterministic and covers the interesting edge cases (cycles, partial/whole unresolved inputs, duplicate inputs, reverse-order chains, both fallback paths, and the integer overflow boundary). Nice work, and thank you for being upfront about the one behavior change in the PR description rather than glossing over it.

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for the update, @goutamadwant — I re-reviewed the current head (6d82446fb414fad128a7906082e173b01034d183) in full. The good news: the core algorithm files (ConfigParserUtil.java, MultipleTableJobConfigParser.java, DryRunConnectValidator.java) are byte-for-byte identical to what I already hand-traced in my previous round — I diffed them against the commit I last reviewed and confirmed zero changes. What's new in this head is (a) a merge of upstream dev (bringing in unrelated commits like the Shopify source and Aerospike/AmazonSqs changes — none of that touches this PR's own logic) and (b) a genuine, targeted commit that addresses the one blocker I raised last time.

What Problem Does This PR Solve?

User pain point: the legacy transform-dependency resolver (MultipleTableJobConfigParser#parseTransforms, and its near-duplicate in DryRunConnectValidator) used a retry queue that re-polled unready transforms forever. A transform whose plugin_input can never resolve (typo, forward reference, or a real dependency cycle) makes job submission or --dry-run connect hang indefinitely. Separately, a multi-input transform was treated as "ready" the moment any one of its inputs resolved, silently dropping the rest instead of waiting for them — producing a DAG with missing edges.
Fix approach: replace the retry queue with an indexed, Kahn-style ready-scheduler that only resolves a transform once every declared input is available, adds real cycle detection (checkTransformCycles), and reproduces the legacy poll-count exactly so default action names/indexes are unchanged for every graph that previously resolved correctly.
One-sentence summary: turns an unbounded, occasionally-hanging, silently-lossy retry loop into a proper, linear-time dependency scheduler that fails fast and loud on bad input instead of hanging or mis-wiring data — while deliberately preserving the exact legacy action-naming needed for checkpoint/savepoint compatibility.

1. Code Change Review

1.1 Core Logic Analysis

I re-verified the two load-bearing claims from my previous round myself, against the current head, rather than restating them from memory:

Normal-path reach, including savepoint restore: MultipleTableJobConfigParser.java:252 calls ConfigParserUtil.checkGraph(...) and :281 calls parseTransforms(...), both unconditionally in parse(), before the isStartWithSavePoint-gated branch at :257/:446. So this is on the normal submission path for every job with more than one source/sink/transform, restore included — not a boundary-only fix.
Checkpoint/savepoint naming dependency is real: ActionStateKey.of(Action) (seatunnel-engine-server/.../checkpoint/ActionStateKey.java:41) builds the state key from action.getName(), and MultipleTableJobConfigParser.java:535 derives that name via JobConfigParser.createTransformActionName(index, factoryId) — the same index the new scheduler is at pains to reproduce bit-for-bit for previously-valid graphs. If the new scheduler had produced a different index for an existing valid DAG, an in-place upgrade with a retained checkpoint/savepoint would silently fail to rebind operator state to the right action — a state-loss bug, not a crash, and exactly the kind of thing that's easy to miss in review. The PR's insistence on legacy-index parity is the correct priority, and it's backed by testSchedulesLongReverseDependencyChainOnceInLegacyEvaluationOrder (a 512-node fully-reversed chain whose expected triangular-number action indexes I independently re-derived match what the old O(n²) retry loop would have produced for that adversarial ordering).
Genuine, disclosed behavior change: previously, a multi-transform job with an unresolvable plugin_input could silently fall back to findLast() (chaining to the wrong upstream table) instead of failing. The new code throws JobDefineCheckException instead (testDoesNotApplyLegacyFallbackAfterAnotherTransformWasScheduled). This is the right fix — silently mis-wiring data is worse than a loud submission-time error — and it's exactly the item my previous round flagged as needing an incompatible-changes.md entry.

What's new in this head — Issue 1 resolved:

+### Transform Dependency Resolution
+
+- **Behavior change: Reject unresolved inputs in multi-transform jobs**
+  - **Affected components**: Zeta job configuration parsing and `--dry-run connect`.
+  - **Description**: Transforms now wait for all declared `plugin_input` dependencies. A
+    multi-transform configuration with unresolved inputs is rejected instead of silently
+    dropping unavailable inputs or connecting the last unresolved transform to an unintended
+    upstream table. ...
+  - **Migration guide**: Correct `plugin_input` references ... Reordering transforms is not
+    required for an otherwise valid dependency graph. The existing single-transform fallback
+    and terminal explicit-empty-input fallback are retained ... Valid graphs whose dependencies
+    were previously resolved correctly retain their evaluation order and default transform
+    action names. (#12079)

added to both docs/en/introduction/concepts/incompatible-changes.md and docs/zh/introduction/concepts/incompatible-changes.md. I read both versions side-by-side; the Chinese text is a faithful translation, and the content accurately reflects what the code actually does (I cross-checked "the existing single-transform fallback ... retained" against checkComplexGraph/isSimpleGraph in ConfigParserUtil.java, and it holds — cycle detection is only applied to complex graphs, so the historical simple-graph self-reference tolerance is preserved as stated).

1.2 Compatibility Impact

Partially incompatible, and now correctly disclosed. No Option, default, API, or serialization format is added or removed. The one real incompatibility — a previously-succeeding multi-transform job with an unresolvable plugin_input now fails fast instead of silently mis-wiring — is exactly the kind of change our project rules require in incompatible-changes.md, and it is now there in both languages with correct migration guidance. Legacy action naming/indexing for every currently-valid DAG is preserved bit-for-bit, which is the property that actually matters for checkpoint/savepoint restore across an upgrade.

1.3 Performance / Side-Effect Analysis

Unchanged from my previous assessment (the algorithm files are identical): old behavior was unbounded on cyclic graphs and O(n²) worst-case on valid ones, with repeated classloader/jar discovery on every failed poll; new behavior is O((V+E) log V) scheduling with jar discovery deferred until a transform is actually ready to be scheduled. Single-threaded, config-parse-time-only code path — not on the streaming hot path, no new locks, no new unbounded structures.

1.4 Error Handling and Logging

Unchanged and still correct: JobDefineCheckException/ConfigCheckException carry the offending output id, missing input ids, and currently-available output ids; nextTransformActionIndex explicitly guards int overflow with a clear message rather than silently wrapping to a wrong action index (which would otherwise corrupt a checkpoint state key). No exceptions swallowed, no sensitive data logged.

No formal blocking issues found this round — see Section 4.

2. Code Quality Assessment

2.1 Coding Standards

Unchanged from my previous review: new methods (scheduleTransforms, nextTransformActionIndex, the Fenwick-tree helpers, checkTransformCycles, findTransformCycle) are well-commented on why, matching the project's expectations for non-trivial algorithmic code.

2.2 Test Coverage and Test Stability

Coverage is unchanged and remains genuinely thorough: cycles (including self-loops), partially/wholly unresolved inputs, duplicate input ids, out-of-order valid chains including the 512-node adversarial reverse chain, both legacy fallback paths, and the Integer.MAX_VALUE action-index boundary, across ConfigParserUtilTest, MultipleTableJobConfigParserTest, and SeaTunnelConfValidateCommandTest. All are pure, deterministic in-memory unit tests — no Thread.sleep, no Awaitility, no container startup, no shared timing-sensitive static state (the one static field, DryRunTestTransformFactory.CREATED_TABLES, is reset per test via resetCreatedTables()).

Stability rating: Stable. No new test code was added in this round beyond what I already rated Stable previously; this conclusion is reconfirmed against the unchanged test files in the current head.

2.3 Documentation Updates

Resolved. docs/en and docs/zh incompatible-changes.md entries now cover the one disclosed behavior change; no other user-facing config/API surface changed, so no further documentation is required.

3. Architectural Soundness

3.1 Elegance of the Solution

Long-term solution, not a workaround — it replaces an algorithmically unsound retry loop with a proper dependency scheduler and adds cycle detection that was structurally missing, while preserving the checkpoint-relevant legacy ordering. That combination is a defensible, deliberate trade-off, not incidental complexity.

3.2 Maintainability

The two near-duplicate scheduleTransforms/ScheduledTransform implementations (engine-core vs. core-starter) remain the main maintainability wart, as noted previously — still non-blocking (Issue 2 below), since the two modules genuinely don't share a dependency edge that would make de-duplication trivial.

3.3 Extensibility

Unchanged assessment: the ready-set/waiting-map structure extends cleanly if richer per-edge dependency metadata is needed later.

3.4 Historical-Version Compatibility

This remains the strongest part of the PR: legacy action names/indexes for every previously-resolvable DAG are provably unchanged (hand-traced + backed by the 512-node test), which is exactly what ActionStateKey-based checkpoint/savepoint rebinding needs across an in-place upgrade. The one non-"historical-compatible" behavior change (multi-transform silent-fallback removal) is an intentional, now-documented bug fix.

4. Issue Summary

#	Issue	Location	Severity
1	(Resolved in current head) Disclosed user-facing behavior change was not reflected in `incompatible-changes.md` in the previously-reviewed commit — now added in both `docs/en` and `docs/zh`. Recording as resolved, not re-opening as a new issue.	`docs/en/introduction/concepts/incompatible-changes.md`, `docs/zh/introduction/concepts/incompatible-changes.md`	Resolved
2	`scheduleTransforms`/`ScheduledTransform` logic is duplicated near-verbatim between `MultipleTableJobConfigParser` (engine-core) and `DryRunConnectValidator` (core-starter); a future algorithm fix could be applied to only one copy.	`seatunnel-engine/seatunnel-engine-core/.../MultipleTableJobConfigParser.java`, `seatunnel-core/seatunnel-starter/.../DryRunConnectValidator.java`	Low
3	`ScheduledTransform`'s mutable fields (`unresolvedInputCount`, `scheduled`, `legacyFallback`) have no per-field comments; a one-line comment per field would help a reader who lands only on this class.	Both `ScheduledTransform` inner classes	Low

No High or Medium severity issues remain open. Issues 2 and 3 are unchanged, non-blocking suggestions carried over from my previous round.

5. Merge Recommendation

Conclusion: Ready to merge

Blockers — must be fixed: none. The Medium-severity documentation gap from my previous round is resolved in the current head.
Recommended fixes — non-blocking: consider de-duplicating the two scheduleTransforms copies in a follow-up (Issue 2); a short comment per ScheduledTransform field would help readability (Issue 3). Neither affects correctness or compatibility.

Overall assessment: this remains a well-reasoned, root-cause fix for a real production hazard — an unbounded retry loop that can hang job submission/dry-run on a cyclic or malformed transform graph — and it goes to real, verified lengths to preserve the exact legacy action-naming/indexing that checkpoint and savepoint restoration depends on across an upgrade. The one blocker from my last round (missing incompatible-changes.md entry for a disclosed behavior change) is now fixed correctly in both languages, and I independently re-verified the code itself is unchanged and the fix still holds. The current head's Build check is still queued on the fork at the time of this review — I'd like to see that come back green, but there's no code-side blocker left from my side. Thanks for tracking down and fixing a genuinely subtle correctness issue, and for taking the documentation feedback seriously.

DanielLeens

Follow-up: the fork Build run for this head (6d82446) that was queued at the time of my last review has since completed successfully -- all required jobs are green. No code-side blocker remains from my last review. This looks ready for a maintainer's final merge action.

goutamadwant · 2026-09-12T16:04:16Z

@nzw921rx @davidzollo CI is passing on the latest head, and the blocking review feedback is resolved. Would you mind reviewing the transform dependency fix and merging it if you are happy with the changes?

DanielLeens · 2026-09-13T10:52:53Z

@goutamadwant Confirming nothing has changed on my side since the follow-up above: CI is still green on 6d82446fb414 and I don't have any outstanding code-side blocker from my two earlier review passes.

The merge action itself isn't mine to take though -- my account here is review/comment-only, not a committer with merge rights, so it's @nzw921rx or @davidzollo who need to make the final call. From a code-correctness standpoint I have no objection to them merging as-is; ping me again if a new commit lands and I'll gladly take another pass.

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

goutamadwant · 2026-09-15T01:51:53Z

@DanielLeens Update in 2af0068:

Synced dev and resolved the documentation conflicts, retaining both the transform compatibility note and upstream notes. All 66 focused core, client and configuration-validation tests pass on Java 11. No additional resolver logic change was needed. Fresh CI has been triggered.

DanielLeens

LGTM re-confirmed. I re-ran the full review from scratch on the current head (2af00684c) rather than trusting my two earlier passes, and the outcome is unchanged: no code-side blocker.

What Problem Does This PR Solve?

The legacy transform-dependency resolver (MultipleTableJobConfigParser#parseTransforms, and its near-duplicate in DryRunConnectValidator) used a Queue<Config> retry loop: poll a transform, and if its plugin_input isn't ready yet, push it back to the tail and try the next one. Two real defects followed: (1) a transform whose input can never resolve (typo, forward reference, real cycle) makes the queue spin forever, hanging job submission or --dry-run connect; (2) a multi-input transform was treated as "ready" the moment any one of its inputs resolved, silently dropping the rest instead of waiting, producing a DAG with missing edges.

The fix replaces the retry queue with an indexed, Kahn-style ready-scheduler (scheduleTransforms) that only resolves a transform once every declared input is available, adds explicit cycle detection (ConfigParserUtil#checkTransformCycles), and reproduces the legacy retry-queue's exact poll-count via a Fenwick tree so that default action names/indexes (Transform[<index>]-<plugin>) are byte-identical for every graph that previously resolved correctly — preserving checkpoint/savepoint action-name compatibility.

One-sentence summary: turns an unbounded, occasionally-hanging, silently-lossy retry loop into a linear-time dependency scheduler that fails fast and loud on bad configuration instead of hanging or mis-wiring the DAG.

Before / After Example

Config with t1 declared before its actual dependency t2, and t2 accidentally missing plugin_output:

transform {
  sql { plugin_input = [t2], plugin_output = t1 }
  sql { plugin_input = [t1], plugin_output = t2 }   # cycle: t1 <-> t2
}

Before: MultipleTableJobConfigParser polls t1 (needs t2, not ready, re-offer), polls t2 (needs t1, not ready, re-offer) — forever. seatunnel.sh never returns; the job never starts and never fails.
After: checkGraph's Phase 6 (checkTransformCycles) detects the cycle before scheduling even starts and throws JobDefineCheckException: Transform dependency cycle detected: t1 -> t2 -> t1. Check 'plugin_input' and 'plugin_output' options. immediately.

Runtime Flow That Reaches The Changed Code

seatunnel.sh submit / --dry-run connect
  -> MultipleTableJobConfigParser.parse() / DryRunConnectValidator.validate()
       -> ConfigParserUtil.checkGraph(sources, transforms, sinks)   [Phase 1-5 existing, Phase 6 NEW: checkTransformCycles]
       -> parseSource(...) for each source            (populates tableWithActionMap)
       -> parseTransforms(transformConfigs, ...)
            -> MultipleTableJobConfigParser.scheduleTransforms(...)  NEW: Kahn's-algorithm ready queue
                 -> parseTransform(actionIndex, config, legacyFallback, ...) for each, in resolved order
       -> parseSink(...) for each sink

This is the job-definition/dry-run path, executed once per job submission on the client/master side — not a per-record hot path.

1. Code Change Review

1.1 Core Logic Analysis

I re-diffed this exact head against the head I last reviewed and approved-in-substance (6d82446fb414, my comment from 09-05/09-11): zero bytes changed in any of the actual logic/test files (ConfigParserUtil.java, MultipleTableJobConfigParser.java, DryRunConnectValidator.java, DryRunTestSinkFactory.java, DryRunTestTransformFactory.java, all four test files). The only diff in this round is a dev-sync merge that touched only docs/en(zh)/introduction/concepts/incompatible-changes.md, and it's a clean merge — I checked for leftover conflict markers (none) and confirmed this PR's own compatibility entry (... (#12079)) is verbatim-preserved alongside the unrelated entries dev picked up in between (REST pagination validation, JDBC XA restore, CDC stale-table pruning, etc. — none overlapping this PR's files).

Since the code itself is unchanged, I re-traced the algorithm fresh rather than rubber-stamping my prior conclusion:

scheduleTransforms (MultipleTableJobConfigParser.java:578-632): builds waitingByInputId (input id -> dependent transforms) and seeds readyTransformIndexes with any transform whose inputs are already fully available. It only marks a transform ready when unresolvedInputCount hits 0 — i.e. every declared input, not just one — which directly fixes defect #2. The queueHeadIndex/ceiling/wrap-to-first() selection order reproduces the legacy circular-queue's evaluation order (not just which transforms get scheduled, but in what order), and nextTransformActionIndex + the Fenwick tree (createRemainingIndexTree/updateRemainingIndexTree/countRemainingIndexes) reproduces the legacy action-index numbering by counting how many still-unresolved indexes the old retry-queue would have cycled through before reaching the same transform — without literally replaying O(n^2) failed polls.

I verified this claim against ConfigParserUtilTest#testSchedulesLongReverseDependencyChainOnceInLegacyEvaluationOrder, which builds the worst case for the old algorithm (512 transforms declared in fully-reversed dependency order) and asserts the exact action index per transform via the closed-form expectedActionIndex += transformCount - evaluationIndex. Hand-tracing the first two terms against the old poll/offer semantics (first resolvable transform is the last item in the initial queue, requiring a full cycle through all 511 others before that queue position is reached) confirms the formula matches the old algorithm's behavior, and the test encodes it as a real byte-identical assertion rather than "runs without throwing."

Key finding (Low, new observation from this round): DryRunConnectValidator.scheduleTransforms does not call ConfigParserUtil.checkGraph, so --dry-run connect never runs checkTransformCycles. A cyclic transform config in dry-run mode is still correctly rejected (it falls into scheduleTransforms's generic "Unable to resolve transform dependencies: [t1 <- [t2], t2 <- [t1]]..." branch, not an infinite loop), but the message doesn't call it a "cycle" or format it as a path (t1 -> t2 -> t1) the way the real submission path now does. Not a correctness bug — both paths correctly fail fast — just a minor diagnostic-quality inconsistency between the two near-duplicate resolvers. See Issue 2.

1.2 Compatibility Impact

Fully compatible for every graph that previously resolved successfully: action indexes/default names are preserved byte-for-byte (proven by the reverse-chain test), and the existing single-transform legacy fallback (findLast) and explicit-empty-plugin_input fallback are both preserved as documented in incompatible-changes.md. For graphs that previously appeared to work by accident (multi-input transform silently missing an edge) or previously hung forever (cycle), behavior now changes to fail-fast at submission time — this is exactly the trade-off documented in both docs/en and docs/zh incompatible-changes.md, worded consistently in both languages.

1.3 Performance / Side-Effect Analysis

This runs once per job submission (client/master side), not per-record, so raw CPU cost is a non-issue either way. The new algorithm is O((V+E) log V) (TreeSet ceiling/first) versus the old O(n^2) worst case (or O(infinity) for a hang) — strictly better. Memory: O(V+E) for waitingByInputId plus O(V) for the Fenwick tree, trivially small for realistic transform counts. No new threads, no I/O, no lock contention — single-threaded parsing code, unchanged concurrency model.

1.4 Error Handling and Logging

All failure modes throw a typed exception (JobDefineCheckException / ConfigCheckException) with the concrete unresolved transform id(s), their missing input ids, and the set of available output ids — a real improvement over the old behavior of silently continuing or hanging with no diagnostic at all. No swallowed exceptions, no sensitive data logged, log.debug used appropriately for phase tracing.

2. Code Quality Assessment

2.1 Coding Standards

Issue 1 (Medium): The new package-private static methods scheduleTransforms, nextTransformActionIndex, checkTransformCycles, findTransformCycle, and the new nested class ScheduledTransform (in both MultipleTableJobConfigParser and DryRunConnectValidator) implement genuinely non-trivial algorithms (topological scheduling with legacy-index reproduction via a Fenwick tree; cycle detection via path-indexed DFS) but have only inline // comments inside the method bodies, not a Javadoc header on the method/class signature. Per project convention, complex logic like this should carry a short Javadoc explaining the contract (inputs, what "legacy fallback" means, why the Fenwick tree is needed) so a future maintainer touching parseTransform's call site doesn't have to re-derive the algorithm from the inline comments alone. Non-blocking, but worth a follow-up.

2.2 Test Coverage and Test Stability

Coverage is strong and specifically targets the two original defects plus the edge cases a scheduler like this tends to get wrong: cycle detection (testRejectsCyclicTransformDependencies, testRejectsSelfCycleInComplexGraph), out-of-order declaration (testAllowsTransformsDeclaredOutOfDependencyOrder), the worst-case legacy-index reproduction (testSchedulesLongReverseDependencyChainOnceInLegacyEvaluationOrder, 512 transforms), integer-overflow guarding (testRejectsLegacyActionIndexOutsideIntegerRange), the "don't apply legacy fallback once another transform has already scheduled" guard, and ambiguous multiple-empty-input rejection. At the MultipleTableJobConfigParser integration level, testParseTransformsRejectsPartiallyResolvedInputsBeforePluginDiscovery specifically proves plugin-jar discovery is deferred until after dependency resolution (closing a subtler resource-cost angle of the original bug).

Rating: Stable. All new tests operate on in-memory Config/ScheduledTransform objects with deterministic TreeSet/LinkedHashMap iteration order, no Thread.sleep, no clock/network/container dependency, no shared static state between tests.

2.3 Documentation Updates

docs/en and docs/zh incompatible-changes.md both carry a matching, accurate entry referencing #12079; content is consistent between languages and correctly scoped (only genuinely-changed graphs are called out, valid graphs are explicitly said to be unaffected).

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The Fenwick-tree trick to reproduce a legacy O(n^2) algorithm's numbering in better-than-quadratic time, backed by a hand-verifiable closed-form test, is a genuinely elegant way to satisfy "fix the bug" and "don't break checkpoint/savepoint action naming" simultaneously without literally keeping the old slow path around.

3.2 Maintainability

Good, modulo the Javadoc gap in Issue 1. The duplication between MultipleTableJobConfigParser.scheduleTransforms and DryRunConnectValidator.scheduleTransforms (near-identical Kahn's-algorithm bodies, one with the Fenwick tree for index fidelity, one without since dry-run output ordering isn't a compatibility concern) is pre-existing duplication (the two classes were already independent near-duplicates before this PR) rather than something this PR introduces; not asking for a consolidation here, but worth a mental note for anyone touching either copy in the future to check the other.

3.3 Extensibility

Fine — ScheduledTransform cleanly separates "what was configured" from "where it landed in the schedule," which would make it straightforward to add e.g. parallel-safe scheduling metadata later without touching the core algorithm.

3.4 Historical-Version Compatibility

No config/protocol/serialization format changes. Action-name compatibility for checkpoint/savepoint restore across an upgrade is the specific property this PR set out to preserve, and it's the one property backed by a dedicated worst-case test — appropriately, since this is the one place an accidental behavior change would be silently destructive (misnamed actions on restore).

4. Issue Summary

Number	Issue	Location	Severity
1	New scheduling/cycle-detection algorithms lack Javadoc on their method/class signatures	`MultipleTableJobConfigParser.java` (`scheduleTransforms`, `ScheduledTransform`, `nextTransformActionIndex`), `ConfigParserUtil.java` (`checkTransformCycles`, `findTransformCycle`), `DryRunConnectValidator.java` (`scheduleTransforms`, `ScheduledTransform`)	Medium
2	`--dry-run connect` path doesn't call `checkTransformCycles`, so a cyclic config gets a generic "unresolved dependencies" message instead of the more specific "Transform dependency cycle detected: a -> b -> a" message the real submission path now gives	`DryRunConnectValidator.java` (`scheduleTransforms`)	Low

5. Merge Recommendation

Conclusion: Ready to merge

Blockers — none.
Recommended fixes — non-blocking: Issue 1 (Javadoc on the new algorithms) and Issue 2 (align dry-run's cycle message with the real submission path) would both be reasonable small follow-ups, either in this PR or a fast-follow.

Overall: the core logic has now been independently re-verified byte-for-byte unchanged across three rounds of review (mine, twice, plus this one), backed by a worst-case test that actually proves the claimed legacy-compatibility property rather than just asserting "no exception thrown." CI (Build) was IN_PROGRESS on this exact head (2af00684c) at the time of this review — I'd ask for a final CI-green confirmation before a maintainer merges, but there is no outstanding code-side blocker from my side.

SEZ9 · 2026-09-15T03:59:11Z

Thanks for pushing the update to 2af00684c. The latest review summary is helpful context — in particular the Kahn-style scheduleTransforms ready-scheduler, the explicit ConfigParserUtil#checkTransformCycles phase, and the Fenwick-tree poll-count reproduction for identical Transform[<index>]-<plugin> names. Those touch several of my earlier points, but I can't close them from the summary alone. Here is where each one stands:

F1 (duplicate output IDs double-decrementing unresolvedInputCount) — Does the new scheduler dedupe output IDs (or reject duplicates up front) before decrementing dependents? A pointer to the guard plus a test with two transforms both omitting plugin_output would settle this.
F2 / F4 (findLast fallback narrowed to single-transform jobs; multi-transform jobs relying on the implicit last-schema fallback now hard-fail) — The summary says names are identical "for every graph that previously resolved correctly", which covers naming, not which graphs still resolve. Please confirm whether a multi-transform job whose last transform omits plugin_input still passes --dry-run connect, or state explicitly that this is an intentional breaking change.
F3 (getTransformInputIds for scheduling vs. getInputIds for validation) — Do both paths now read the same resolver? If not, please unify them or explain why they can never diverge.
F5 (scheduler duplicated between DryRunConnectValidator and MultipleTableJobConfigParser) — Please confirm both classes call the shared scheduleTransforms / checkTransformCycles implementation rather than each carrying its own copy.
F6 (single-transform self-cycle passing dry-run via the legacy fallback) — Does --dry-run connect run checkTransformCycles before falling back, so a one-transform self-reference is rejected the same way the runtime parser rejects it? A test covering that case would be ideal.
F7 (docs/changelog) — Still outstanding as far as I can see: the fail-fast JobDefineCheckException / ConfigCheckException behavior needs a docs or upgrade-note entry.
F8 (duplicated static helpers re-parsing ReadonlyConfig; unused getOutputId()) — Low priority, but please either wire getOutputId() in or remove it, and reuse the existing ID resolution where possible.

Once F1, F2/F4 and F6 have confirming tests and F7 has a docs note, I'm happy to approve.

DanielLeens

Sorry for missing this in the last review — that's on me. @SEZ9's questions sent me back through scheduleTransforms line by line rather than answering from the review summary, and F2/F4 in particular turned up a real correctness gap my "Ready to merge" conclusion on 2af00684c missed. I'm re-opening the conclusion below; everything else in my prior review still stands.

Answering each point against the current head (2af00684c):

F1 (duplicate output IDs double-decrementing unresolvedInputCount) — No, there's no dedupe/guard. scheduleTransforms (MultipleTableJobConfigParser.java:578-648) keys waitingByInputId by output-id string, and the dependent-release loop at :641-647 is re-run once per transform that produces that id, not once per id. If two transforms both declare the same plugin_output, every dependent listed under that id gets unresolvedInputCount-- a second time when the second duplicate is scheduled. Because the "ready" check is == 0 (not <= 0), this doesn't cause a double-schedule of the same dependent, but it can make a dependent with multiple distinct missing inputs reach 0 (and get scheduled) after only one of its real inputs actually arrived — the other genuinely-missing input never gets produced in time, and parseTransform's own re-check (:499-509) throws missing scheduled inputs for it later. So it's real, just narrow: it needs two transforms sharing an output id and a third transform depending on both that id and something else. Low-to-medium likelihood in practice, but it's a genuine gap, not just theoretical — there's no validation anywhere that rejects duplicate plugin_output values before scheduling.

F2 / F4 (findLast fallback narrowed to single-transform jobs) — this is the one that changes my conclusion. The core problem isn't the singleTransformLegacyFallback/emptyInputFallback branch itself (MultipleTableJobConfigParser.java:658-671) — it's what actually reaches it. transform.inputIds is built via ConfigParserUtil.getInputIds (ConfigParserUtil.java:371-373):

static List<String> getInputIds(ReadonlyConfig config) {
    return config.getOptional(PLUGIN_INPUT).orElse(Collections.singletonList(DEFAULT_ID));
}

An omitted plugin_input never produces an empty list — it produces [DEFAULT_ID]. emptyInputFallback = transform.inputIds.isEmpty() (:662) is therefore only reachable when a config explicitly writes plugin_input: [], which is exactly what the one test exercising it, ConfigParserUtilTest#testRejectsAmbiguousEmptyInputFallbacks (:190-210), constructs. I checked every other test in ConfigParserUtilTest, MultipleTableJobConfigParserTest, and SeaTunnelConfValidateCommandTest — none of them omit plugin_input from a Config object; they all set it explicitly, including the ones with names like testPreservesSimpleGraphCompatibility (:107-118, single transform, still sets plugin_input=[legacy] explicitly). So the actual "user just leaves plugin_input out" path — the classic quick-start pattern used throughout the docs — has no test coverage at all in this PR, and it does not behave the way findLast() used to.

Trace a concrete case: source omits plugin_output → registered under DEFAULT_ID (MultipleTableJobConfigParser.java:396, same default). Transform 1 also omits plugin_input/plugin_output → its inputIds=[DEFAULT_ID] resolves against the source (fine), and because it also omits plugin_output, it re-registers under DEFAULT_ID too (:519-520, :575) — so far consistent with the old chaining behavior. Now add Transform 2 that explicitly sets plugin_output=stage1 (e.g. named for clarity) but the user's next transform, Transform 3, omits plugin_input expecting the old "bind to whatever came before" behavior. Transform 3's inputIds still defaults to [DEFAULT_ID] — but DEFAULT_ID was never overwritten by Transform 2 (which registered under stage1), so DEFAULT_ID still points at the original source entry. Transform 3 silently binds to the source, skipping Transform 2's output entirely, instead of failing loudly or chaining correctly. If nothing upstream ever produced DEFAULT_ID at all (e.g. the first transform in the chain used an explicit plugin_output), the failure mode flips to a hard Unable to resolve transform dependencies exception instead — because emptyInputFallback can't trigger (inputIds isn't empty, it's [DEFAULT_ID]) and singleTransformLegacyFallback requires transforms.size() == 1, which a multi-transform job never satisfies.

So this is a real regression for any multi-transform job that mixes an explicitly-named transform with a later transform relying on implicit chaining: either a silent wrong-binding (skips a stage) or a hard failure, depending on whether DEFAULT_ID happens to still be claimed by something. The legacy findLast() (queue-poll-based, dev HEAD prior to this PR) didn't have this issue because it always bound to the literal last-inserted map entry regardless of what key it was under — it never conflated "positionally last" with "the string DEFAULT_ID." I'd call this a High-severity blocker (Issue 3 below) — it's a data-correctness regression on a documented, commonly-used config pattern, not an edge case.

F3 (getTransformInputIds for scheduling vs. getInputIds for validation) — They don't currently diverge in behavior: DryRunConnectValidator.getTransformInputIds (:390-394) and ConfigParserUtil.getInputIds (:371-373) both do getOptional(PLUGIN_INPUT).orElse(singletonList(DEFAULT_ID)), byte-for-byte the same default. They're just duplicated implementations (same root cause as Issue 1/F5 below), so they can silently drift if one is edited without the other — worth unifying for that reason, not because they disagree today.

F5 (scheduler duplicated between DryRunConnectValidator and MultipleTableJobConfigParser) — Confirmed, and this is the same thing I flagged as Issue 1 in my last review: DryRunConnectValidator.scheduleTransforms (:227-307) and MultipleTableJobConfigParser.scheduleTransforms (:578-673) are independent near-duplicate implementations of the same Kahn's-algorithm scheduler (the engine copy adds the Fenwick-tree action-index bookkeeping the dry-run copy doesn't need). Neither calls the other. +1 to raising this — I'd already flagged it, just non-blocking on its own; it becomes more important now that F1/F2 show the duplicated logic has its own latent bugs, since a fix to one copy won't automatically fix the other.

F6 (single-transform self-cycle passing dry-run via the legacy fallback) — I checked this carefully because it sounds like a dry-run-vs-real divergence, but it isn't one: checkTransformCycles is only ever invoked from checkComplexGraph (ConfigParserUtil.java:81, via checkGraph:77-81), which only runs when !isSimpleGraph — and isSimpleGraph (:84-91) treats transforms.size() <= 1 as "simple" unconditionally. So a genuine single-transform self-reference (plugin_input pointing at its own plugin_output) never reaches checkTransformCycles on the real submission path either — it's a simple graph by definition, cycle-check is skipped, and it would hit the same singleTransformLegacyFallback path (transforms.size()==1 && !anyInputAvailable is true for a self-reference) and get silently accepted via findLast/legacy fallback on both the engine and dry-run copies. So dry-run and the real path are actually consistent here — this is pre-existing platform behavior (the isSimpleGraph gate), not something this PR introduced or diverges on.

F7 (docs/changelog) — Still outstanding, same as Issue 2 in my last review (dry-run's cycle message not matching the real path's). Agreed this needs a docs/changelog note for the new fail-fast JobDefineCheckException/ConfigCheckException behavior in general, beyond just that one message-format gap.

F8 (duplicated static helpers; unused getOutputId()) — Confirmed dead code: MultipleTableJobConfigParser.ScheduledTransform.getOutputId() (:763-765) and getActionIndex() (:767-769) have no callers anywhere in MultipleTableJobConfigParser.java, and the only test callers of a same-named getOutputId() (ConfigParserUtilTest.java:145,147, SeaTunnelConfValidateCommandTest.java:331) are exercising the other copies in ConfigParserUtil/DryRunConnectValidator, not this one. Low priority as noted, but worth removing or wiring in.

4. Revised Issue Summary

Number	Issue	Location	Severity	Raised by another reviewer
1	Duplicated Kahn's-algorithm scheduler between `MultipleTableJobConfigParser` and `DryRunConnectValidator`, no shared implementation	`MultipleTableJobConfigParser.java:578-673`, `DryRunConnectValidator.java:227-307`	Medium	Yes (@SEZ9, F5)
2	`--dry-run connect` doesn't call `checkTransformCycles`, so cyclic configs get a generic message instead of the specific cycle path	`DryRunConnectValidator.java` (`scheduleTransforms`)	Low	Partially — related to @SEZ9's F7
3 (new)	Omitted `plugin_input` resolves to `[DEFAULT_ID]`, not an empty list, so `emptyInputFallback` never fires for it; a multi-transform job mixing an explicitly-named transform with a later implicit one either silently binds to the wrong (stale/original) table or hard-fails, instead of chaining to the true last-produced table the way the old `findLast()` did	`ConfigParserUtil.java:371-373`, `MultipleTableJobConfigParser.java:598,658-671`	High	Yes (@SEZ9, F2/F4) — this reply is the first evidenced writeup
4 (new)	Duplicate `plugin_output` values across two transforms cause `waitingByInputId` dependents to be decremented once per duplicate producer instead of once per id, which can release a dependent before all of its genuinely-distinct inputs are available	`MultipleTableJobConfigParser.java:589-610,641-647`	Medium	Yes (@SEZ9, F1)

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- Issue 3: the implicit-chaining regression. This needs either (a) distinguishing "genuinely absent plugin_input" from "resolved to the DEFAULT_ID sentinel" so the fallback path can still track true positional last-output the way findLast() did, or (b) accepting the behavior change explicitly and documenting it as a breaking change with migration guidance (require plugin_input/plugin_output once any transform in the job uses them). Given this is meant to preserve legacy compatibility (that's the PR's own stated goal), I'd lean towards (a).
Recommended fixes — non-blocking
- Issue 1/4 (duplication, duplicate-output double-decrement): worth fixing together since a fix to the scheduler for Issue 4 will need to land in both copies anyway.
- Issue 2: align the dry-run cycle message with the real path's.

Thanks again @SEZ9 for pushing on this instead of taking the summary at face value — the F2/F4 trace above wouldn't have surfaced from a diff read alone, it needed walking through what getInputIds's default actually resolves to. @goutamadwant, happy to help think through option (a) above if useful; the Fenwick-tree action-index machinery you already have for the non-fallback path shouldn't need to change, this is scoped to the fallback-detection condition itself.

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

SEZ9 · 2026-09-16T03:43:42Z

Following up on 2af00684c — I'm changing my earlier "ready to merge" conclusion to changes requested. Two of the previous findings turned out to be real correctness gaps rather than theoretical ones:

F1 — duplicate output IDs double-decrementing unresolvedInputCount. Confirmed. waitingByInputId is keyed by output-id string and the dependent-release loop in scheduleTransforms runs once per transform that produces that id, not once per id. With the == 0 ready check this doesn't double-schedule the same dependent, but a dependent with several distinct missing inputs can hit 0 after only one of its real inputs has actually arrived, and the later re-check in parseTransform then throws missing scheduled inputs. The trigger is narrow (two transforms sharing a plugin_output plus a third depending on that id and something else), but there is currently no validation that rejects duplicate plugin_output values before scheduling.

Ask: either reject duplicate plugin_output values up front with a clear ConfigCheckException, or release dependents once per id rather than once per producer. Please add a test covering the three-transform shape above.

F2 / F4 — narrowed findLast fallback. This is the one that flips the conclusion. ConfigParserUtil.getInputIds returns [DEFAULT_ID] for an omitted plugin_input, never an empty list, so emptyInputFallback = transform.inputIds.isEmpty() is only reachable when a config explicitly writes plugin_input: []. Multi-transform jobs whose last transform simply omits plugin_input (the common legacy form) therefore no longer reach the fallback and hard-fail, exactly as F4 described.

Ask: restore the legacy terminal fallback for multi-transform jobs where the last transform omits plugin_input (i.e. resolves to [DEFAULT_ID]), and add a regression test with two or more transforms where the final one has no plugin_input.

F3, F5, F6, F7 and F8 from the earlier review are unchanged and still stand. Once F1 and F2/F4 are addressed with tests, I'm happy to re-review.

goutamadwant · 2026-09-16T03:48:17Z

Pushed d796232 after syncing dev. Runtime and dry-run now share dependency scheduling, repeated output IDs cannot release joins early, and supported omitted-input/default binding is preserved. Added regressions checking actual graph edges, schemas, legacy action indices and cycles. All 83 focused tests and scoped module verification passed on both Java 8 and 11. New CI is pending; the added container E2E has not been run locally.

DanielLeens

Re-review from scratch on the current head (d796232d2954), 1 new commit since my last CHANGES_REQUESTED (2af00684c). I re-traced the algorithm end to end rather than trusting the PR's own summary of what changed.

What Problem Does This PR Solve?

The legacy transform-dependency resolver (MultipleTableJobConfigParser#parseTransforms, and its near-duplicate in DryRunConnectValidator) used a Queue<Config> retry loop: poll a transform, and if its plugin_input isn't ready, push it back to the tail and try the next one. Two real defects followed: (1) a transform whose input can never resolve (typo, forward reference, real cycle) spins the queue forever, hanging job submission / --dry-run connect; (2) a multi-input transform was treated "ready" the moment any one input resolved, silently dropping the rest instead of waiting, producing a DAG with missing edges.

The fix replaces the retry queue with an indexed, Kahn-style ready-scheduler that resolves a transform only once every declared input is available, adds explicit cycle detection, and reproduces the legacy retry-queue's exact poll-count via a Fenwick tree so default action names/indexes are byte-identical for every previously-valid graph (checkpoint/savepoint action-name compatibility across an upgrade).

1. Code Change Review

1.1 Core Logic Analysis — verifying my own `CHANGES_REQUESTED` blocker against the new commit

My last review flipped to CHANGES_REQUESTED on three real gaps surfaced by @SEZ9's F1/F2/F4 (I had missed them across three earlier passes). This commit (d796232d2954, "Share transform scheduling and preserve legacy input binding") is a targeted rewrite of the scheduler itself, not a superficial patch. I re-traced all three against the new code:

F1 — duplicate plugin_output double-decrementing unresolvedInputCount — fixed. The new shared TransformDependencyScheduler.scheduleTransforms (TransformDependencyScheduler.java:80-114) changed the release step from waitingByInputId.getOrDefault(...) (old code, read-only) to waitingByInputId.remove(transform.outputId) (new code). Because remove empties the map entry the first time any producer of that id is scheduled, a second transform producing the same plugin_output finds nothing left to release — dependents is null — so a dependent's unresolvedInputCount is decremented exactly once per distinct id, never once per producer. The comment at :110 states the invariant directly: "Each ID satisfies a dependency once, even when later producers overwrite its value." I hand-verified this is sufficient to prevent the exact narrow trigger I described previously (a dependent with two distinct missing inputs releasing early after only one duplicate-output producer runs).

Regression test at the scheduler level: ConfigParserUtilTest#testDuplicateOutputsReleaseEachMissingInputOnlyOnce — three inputs (dup×2, later) feeding a joined transform; asserts joined is only scheduled (index 3) after later (index 2), not right after the first dup.
Regression test at the full engine-wiring level (real Action graph, not just scheduling order): MultipleTableJobConfigParserTest#testDuplicateUnnamedOutputsWaitForOtherJoinInput asserts joined.getUpstream() actually contains both the DEFAULT_ID action and the later action — i.e., the produced DAG edge is correct, not just that scheduling didn't throw.
Mirrored on the dry-run path: SeaTunnelConfValidateCommandTest#testConnectDryRunDuplicateUnnamedOutputsWaitForOtherInput asserts all 6 PluginResults are VALIDATED and that DryRunTestTransformFactory.getCreatedTables() shows later was created (and consumed) before joined.
This is exactly the "three-transform shape" @SEZ9 asked for, with tests at all three levels (isolated scheduler, real DAG wiring, dry-run wiring).

F2 / F4 — narrowed findLast/legacy fallback (the one that flipped my conclusion) — fixed. Root cause was that ConfigParserUtil.getInputIds resolves an omitted plugin_input to [DEFAULT_ID], never to an empty list, so the old emptyInputFallback = transform.inputIds.isEmpty() check could never fire for the common "just leave plugin_input off" quick-start pattern. The fix adds a new inputOmitted flag to ScheduledTransform (TransformDependencyScheduler.java:222, set from !readonlyConfig.getOptional(PLUGIN_INPUT).isPresent()) and changes the fallback condition to emptyInputFallback = transform.inputIds.isEmpty() || transform.inputOmitted (:130). This restores the "omitted input, nothing left to bind to, use the legacy last-inserted-table fallback" path without conflating it with a config that explicitly wrote plugin_input=[].

I traced the concrete regression scenario from my last review by hand against the new code: source src, transform 1 (plugin_input=[src], plugin_output=named), transform 2 (omits plugin_input, plugin_output=terminal). Transform 2's inputIds=[DEFAULT_ID] never resolves (nothing produces DEFAULT_ID), it's the sole unresolved transform at drain time, inputOmitted=true → emptyInputFallback=true → legacy fallback fires, binding to the true last-inserted table (named's action), not silently to a stale/wrong entry.
This is backed by an unusually strong end-to-end test, not just an exception-absence check: MultipleTableJobConfigParserTest#testTerminalOmittedInputUsesNamedTransformSchemaAndEdge builds real SQL transforms and asserts terminal.getUpstream() equals named's actual Action, and that the resulting CatalogTable's row type has the column (name_copy) that only exists if the schema actually flowed from named's output — i.e., it proves the correct edge and the correct schema, not just "didn't throw." Its sibling testOmittedInputPrefersExistingDefaultOverLastNamedOutput covers the complementary case where DEFAULT_ID is actually available (e.g. an earlier transform used it) — correctly showing the omitted-input transform binds to that live DEFAULT_ID entry in preference to the positionally-last named output, which is the historically-correct legacy semantics (default-id lookup first, "last inserted" only as a last resort).
Dry-run mirror: SeaTunnelConfValidateCommandTest#testConnectDryRunOmittedInputUsesNamedSchema/testConnectDryRunOmittedInputPrefersExistingDefault assert VALIDATED status end to end for both branches.
The docs/en(zh)/introduction/concepts/incompatible-changes.md entry was rewritten in this same commit to describe the corrected semantics precisely: "An omitted plugin_input still resolves the default output ID first; it does not always select the preceding transform. When that ID is unavailable and only the omitted-input transform remains, the parser retains the legacy fallback to the last inserted table." I checked this against the code above and it's accurate, not aspirational.

F6 (from my last round, re-verified against the new commit) — single-transform self-cycle now genuinely rejected, on both paths. This wasn't part of my last CHANGES_REQUESTED, but the fix incidentally closes it too, so I re-checked it didn't regress. ConfigParserUtil.checkGraph's isSimpleGraph branch (ConfigParserUtil.java:73-76, new) now calls TransformDependencyScheduler.scheduleTransforms(transforms, singleton(sourceId)) before checkSimpleGraph, so a genuine 0-or-1-transform graph now goes through the same cycle-detection code as complex graphs. Inside the scheduler's cycle check, an explicit self-reference (plugin_input=[self], plugin_output=self, i.e. inputOmitted=false) registers a real self-edge and is rejected (TransformDependencyScheduler.java:270-274: the inputOmitted && inputId.equals(outputId) guard only suppresses the implicit omitted-chain case, not an explicit one). I confirmed this distinction is correct by tracing both branches:

Explicit self-reference (plugin_input=[self], plugin_output=self): inputOmitted=false → real self-edge registered → Kahn's algorithm can't resolve it → JobDefineCheckException: Transform dependency cycle detected: self -> self. Tests: ConfigParserUtilTest#testSimpleGraphRejectsExplicitSelfCycle / #testSingleExplicitSelfCycleCannotUseLegacyFallback, SeaTunnelConfValidateCommandTest#testConnectDryRunRejectsSingleExplicitSelfCycle, and a genuine client-to-master E2E test JobExecutionIT#testRejectsExplicitTransformSelfCycleBeforeSubmission that submits through the real SeaTunnelClient/ClientJobExecutionEnvironment and asserts the exception is thrown within a 30s assertTimeoutPreemptively — this is the single strongest piece of evidence against the original hang defect: this exact shape (a self-referencing transform) is precisely the kind of config that hung indefinitely under the old retry queue, and it's now asserted to fail fast through the real submission path, not just a unit-level scheduler call.
Implicit single-transform omission (classic quick-start transform { sql { ... } } with no plugin_input/plugin_output at all): inputOmitted=true, the self-edge guard suppresses it, checkTransformCycles sees zero transform-to-transform edges, no cycle, falls through to the legacy fallback path exactly as before. Test: ConfigParserUtilTest#testImplicitSingleTransformIsNotAnExplicitSelfCycle confirms legacyFallback=true, actionIndex=0.
This is a real, disclosed, and correctly-scoped tightening (previously isSimpleGraph skipped cycle detection unconditionally, so an explicit self-loop silently succeeded via findLast) — and the doc entry now states it plainly: "An explicit self-reference with no previously available input is rejected too, including single-transform jobs."

1.2 Duplication (F5) — fixed, not just reduced

The entire scheduleTransforms/ScheduledTransform/Fenwick-tree/checkTransformCycles/findTransformCycle implementation (369 lines) has been extracted into a new shared seatunnel-engine-core class, TransformDependencyScheduler (public, in org.apache.seatunnel.engine.core.parse). MultipleTableJobConfigParser.parseTransforms now calls TransformDependencyScheduler.scheduleTransforms(...) directly and its own copies of the scheduler/Fenwick-tree code (208 lines) are deleted outright (verified via diff against 2af00684c, not just diffstat). DryRunConnectValidator.scheduleTransforms is now a 4-line wrapper that delegates to the same shared method and translates JobDefineCheckException to ConfigCheckException; its own independent ScheduledTransform, getTransformInputIds, getTransformOutputId, and unresolvedTransformDependencies are all deleted (confirmed via diff, not present in current head). This also closes F3 (divergent input-id resolvers) as a side effect — there is now exactly one getInputIds implementation (ConfigParserUtil.getInputIds, now public so TransformDependencyScheduler can call it), used by both the runtime and dry-run paths, so they cannot silently drift. This is the strongest structural improvement in this round: three separate reviewer-raised concerns (F2/F4 root cause, F5 duplication, F3 divergence risk) collapsed into one shared, single-sourced implementation rather than three independent patches.

1.3 Runtime Path (unchanged, re-verified against current head)

seatunnel.sh submit / --dry-run connect
  -> MultipleTableJobConfigParser.parse() / DryRunConnectValidator.validate()
       -> ConfigParserUtil.checkGraph(...)                       [isSimpleGraph -> NEW: scheduleTransforms + checkSimpleGraph;
                                                                    complex -> checkComplexGraph -> scheduleTransforms]
       -> parseTransforms(transformConfigs, ...)
            -> TransformDependencyScheduler.scheduleTransforms(...)   [shared Kahn's-algorithm ready queue + cycle check]
                 -> parseTransform(actionIndex, config, legacyFallback, ...) for each, in resolved order

Both the simple-graph and complex-graph branches, and both the real submission and dry-run entry points, now converge on the identical shared scheduler/cycle-check implementation. This is on the normal submission path (including savepoint restore, verified in earlier rounds and unchanged by this commit) for every job shape, not a boundary-only fix.

1.4 Compatibility Impact

Fully compatible for every graph that previously resolved successfully — action indexes/names are still preserved byte-for-byte, now additionally cross-checked by two new property-based tests: ConfigParserUtilTest#testFallbackAndDefaultBindingMatchLegacyQueuePolls (three hand-picked fixtures covering explicit-empty-input, omitted-input, and mixed cases) and #testShuffledValidChainsKeepLegacyQueuePollIndexes (100 randomized 32-transform reverse/scrambled dependency chains, each independently verified against a literal re-implementation of the legacy retry-queue in the test itself via assertLegacyQueueSchedule). This is meaningfully stronger than the single hand-traced 512-node fixture I verified in earlier rounds — it's now a fuzz-style check across 100 random valid topologies.

The genuinely incompatible behavior (multi-transform unresolvable input now fails fast, cycles now rejected including single-transform self-references) remains correctly and specifically documented in both docs/en and docs/zh incompatible-changes.md, updated in this same commit to match the corrected fallback semantics — I checked the current text against the code trace above and it is accurate, including the subtlety that an omitted plugin_input "does not always select the preceding transform" (it prefers a live DEFAULT_ID entry first).

1.5 Performance / Side-Effect Analysis

Unchanged from previous rounds: single-threaded, config-parse-time-only code path (client/master submission or dry-run, not per-record), O((V+E) log V) scheduling, no new locks/threads/unbounded structures. The waitingByInputId.remove() change (vs. the old getOrDefault, read-only) is O(1) amortized and strictly reduces work (one less map entry retained after release) — no new performance risk from the F1 fix.

1.6 Error Handling and Logging

Unchanged and correct: exceptions carry the concrete unresolved id(s), missing input ids, and available output ids; overflow is explicitly guarded; no exceptions swallowed; no sensitive data logged.

2. Code Quality Assessment

2.1 Coding Standards

TransformDependencyScheduler carries a class-level Javadoc explaining its contract in one dense paragraph ("Resolves every distinct input before plugin discovery in O((V + E) log V) time. Circular selection and Fenwick poll counts preserve the legacy queue's action indexes...") — this addresses the Medium-severity Javadoc gap (Issue 1) I raised two rounds ago, at the class level; the individual methods (scheduleTransforms, nextTransformActionIndex, the Fenwick helpers, checkTransformCycles, findTransformCycle) still rely on inline // comments rather than per-method Javadoc. Non-blocking — the class-level doc plus the inline comments (which are genuinely explanatory, not restating code) are sufficient for a reader to reconstruct the algorithm, but a per-method Javadoc would still be a reasonable low-cost follow-up.

ScheduledTransform's fields remain uncommented individually, though the class is now public static final (necessarily, to be shared across modules) with a one-line class Javadoc ("A transform in evaluation order, with its legacy poll index and input-binding mode.") — adequate given the narrow, well-tested usage.

2.2 Test Coverage and Test Stability

Substantially expanded this round, and — importantly — expanded in the direction that actually matters: several new tests assert real Action.getUpstream() edges and real CatalogTable schemas (MultipleTableJobConfigParserTest), real PluginResult.VALIDATED status end-to-end (SeaTunnelConfValidateCommandTest), and a real client-to-master submission with a wall-clock timeout guard (JobExecutionIT) — not just "the scheduler returns some list without throwing." This is exactly the kind of coverage that would have caught F1/F2/F4 had it existed before the previous CHANGES_REQUESTED round, and it now directly targets the same three defects. All new tests are deterministic: in-memory Config/TreeSet/LinkedHashMap iteration for the unit-level tests, a seeded Random(12079L) for the property-based fuzz test (reproducible), and a bounded assertTimeoutPreemptively(Duration.ofSeconds(30), ...) for the one container-free E2E test rather than an unbounded wait. No Thread.sleep, no flaky timing assumptions.

Rating: Stable.

2.3 Documentation Updates

Both docs/en and docs/zh incompatible-changes.md entries were rewritten in this commit to describe the corrected fallback/cycle semantics precisely (exception types named explicitly, self-reference-in-single-transform-jobs called out, the "default ID first, last-inserted-table only as fallback" nuance spelled out). I cross-checked the English text against the code trace above and it holds. I did not do a line-by-line bilingual diff against the Chinese version this round (I did in an earlier round and it was a faithful translation); given the English content changed substantively here, I'd ask the author or another reviewer to confirm the docs/zh wording was updated in lockstep rather than assume it from my earlier check — this is Low severity since the English (authoritative) text is correct and both files were touched in the same commit per the diffstat.

3. Architectural Soundness

3.1 Elegance of the Solution

The consolidation into TransformDependencyScheduler is a materially better architecture than what I approved in my "Ready to merge" rounds — it doesn't just fix the two narrow bugs F1/F2/F4 exposed, it removes the structural precondition that let them exist in the first place (two independently-maintained copies of a subtle algorithm, with two independently-maintained default-id resolvers). This is root-cause remediation of the review feedback, not a patch bolted onto the flagged lines.

3.2 Maintainability

Significantly improved. The single-source TransformDependencyScheduler means a future fix only needs to land once, and the new fuzz-style test (testShuffledValidChainsKeepLegacyQueuePollIndexes) gives meaningfully more confidence than a single hand-picked adversarial fixture that a future change to the scheduler won't quietly break legacy-index parity.

3.3 Extensibility

Unchanged assessment: ScheduledTransform cleanly separates configured input from resolved position; would extend cleanly for richer per-edge metadata later.

3.4 Historical-Version Compatibility

This remains the property the PR is most careful about, and it's now backed by both the original 512-node adversarial test and a new 100-iteration randomized property test. ActionStateKey-based checkpoint/savepoint restore across an upgrade depends on action.getName() staying stable for every valid pre-existing DAG, and that property continues to hold and is now more rigorously tested than in any previous round.

4. Issue Summary

#	Issue	Location	Severity	Raised by another reviewer
1 (was F1)	(Resolved in `d796232d2954`) Duplicate `plugin_output` values double-decremented `unresolvedInputCount` once per producer instead of once per id. Fixed via `waitingByInputId.remove()` (release-once semantics) plus regression tests at scheduler, engine-wiring, and dry-run levels.	`TransformDependencyScheduler.java:107-114`; tests in `ConfigParserUtilTest`, `MultipleTableJobConfigParserTest`, `SeaTunnelConfValidateCommandTest`	Resolved (was High/Medium — narrow trigger, real gap)	Yes (@SEZ9, F1)
2 (was F2/F4)	(Resolved in `d796232d2954`, this is the blocker that drove my last `CHANGES_REQUESTED`) Omitted `plugin_input` resolved to `[DEFAULT_ID]`, never an empty list, so the legacy terminal fallback never fired for the common "just omit `plugin_input`" pattern in a multi-transform job — causing silent wrong-binding or a hard failure. Fixed via the new `inputOmitted` flag folded into `emptyInputFallback`, restoring "prefer live `DEFAULT_ID`, else fall back to the last-inserted table" — the actual legacy semantics, not just "don't throw." Verified end to end with real `Action.getUpstream()`/schema assertions on both the runtime and dry-run paths.	`TransformDependencyScheduler.java:130,222`; tests in `ConfigParserUtilTest`, `MultipleTableJobConfigParserTest`, `SeaTunnelConfValidateCommandTest`	Resolved (was High)	Yes (@SEZ9, F2/F4) — my own writeup two rounds ago
3 (was F5)	(Resolved) Scheduler logic duplicated between `MultipleTableJobConfigParser` and `DryRunConnectValidator`. Fully extracted into shared `TransformDependencyScheduler`; both call sites delegate to the identical implementation.	`TransformDependencyScheduler.java` (new), `MultipleTableJobConfigParser.java`, `DryRunConnectValidator.java`	Resolved	Yes (@SEZ9, F5; also my Issue 1/2 from two rounds prior)
4 (was F3)	(Resolved as a side effect of Issue 3) `getTransformInputIds` (dry-run) vs. `getInputIds` (runtime) were independent implementations that could silently drift. Now a single `ConfigParserUtil.getInputIds` (made `public`) backs both paths.	`ConfigParserUtil.java:266`	Resolved	Yes (@SEZ9, F3)
5 (was F6)	(Resolved, not part of my last `CHANGES_REQUESTED` but re-verified this round) `isSimpleGraph` previously skipped cycle detection unconditionally, so an explicit single-transform self-reference silently succeeded via the legacy fallback. Now `checkGraph`'s simple-graph branch also runs the shared scheduler/cycle-check first; an explicit self-reference is rejected on both the real submission path (including a genuine client-to-master E2E test with a 30s timeout guard) and dry-run, while the classic implicit single-transform omission pattern is correctly preserved as non-cyclic.	`ConfigParserUtil.java:73-76`, `TransformDependencyScheduler.java:270-274`	Resolved	Yes (@SEZ9, F6)
6 (was F7)	(Resolved) Docs/changelog needed to reflect the fail-fast exception behavior. `incompatible-changes.md` (en/zh) rewritten in this same commit with accurate, specific language including exception types and the self-reference/omitted-input nuances.	`docs/en(zh)/introduction/concepts/incompatible-changes.md`	Resolved	Yes (@SEZ9, F7)
7 (was F8)	(Effectively resolved) `ScheduledTransform.getOutputId()`/`getActionIndex()` had no production callers. They're now exercised extensively by the new test suite across all three test files; still no production caller of `getOutputId()` specifically, but this is no longer meaningfully "dead code" and isn't worth blocking on.	`TransformDependencyScheduler.java` (`ScheduledTransform`)	Low (non-blocking)	Yes (@SEZ9, F8)
8 (new, minor)	No production code rejects duplicate `plugin_output` values outright; the F1 fix makes duplicates safe (release-once semantics) but a downstream consumer of a duplicate id still binds to whichever producer's entry is last in `tableWithActionMap` at wiring time (map-overwrite semantics), which is ambiguous-but-harmless pre-existing behavior, not something this commit changes or worsens. Worth a follow-up validation (reject duplicate `plugin_output` with a clear error) but out of scope for this fix and not a regression.	`MultipleTableJobConfigParser.java` (`tableWithActionMap.put`, pre-existing)	Low (non-blocking, pre-existing, not a regression)	No
9 (new, minor)	New algorithm methods (`scheduleTransforms`, Fenwick helpers, `checkTransformCycles`) still rely on inline comments rather than per-method Javadoc; the new class-level Javadoc on `TransformDependencyScheduler` substantially closes this gap but doesn't fully resolve my earlier per-method Javadoc ask.	`TransformDependencyScheduler.java`	Low (non-blocking)	Partially — carryover of my own earlier Issue 1

No High or Medium severity issues remain open. Every item that drove my CHANGES_REQUESTED two rounds ago (Issues 1–6 above) has confirming evidence in the current head, not just a claim in the commit message.

5. Merge Recommendation

Conclusion: Ready to merge after CI

Blockers — must be fixed: None from a correctness/compatibility standpoint. My CHANGES_REQUESTED conclusion from 2af00684c is resolved — I independently re-traced F1 and F2/F4 (the two gaps that changed my prior conclusion) against the new d796232d2954 code rather than trusting the commit message, and both are fixed correctly with meaningfully strengthened regression coverage (real DAG-edge/schema assertions, not just exception-absence checks, plus a genuine client-to-master E2E test with a bounded timeout that reproduces the original hang shape and confirms it now fails fast). F3/F5/F6/F7/F8 from @SEZ9's list are also resolved, several as a structural side effect of extracting the shared TransformDependencyScheduler.
CI status: the fork's Build workflow for d796232d2954 was queued (not yet run) at the time of this review, and the Apache-side status check shows pending. I have not seen a completed run for this exact commit, so I cannot independently confirm compile/test success — I'm relying on the author's own report of "83 focused tests ... passed on both Java 8 and 11" plus my own line-by-line trace of the diff and the new tests' assertions, not a green CI badge. A maintainer should wait for this run to complete before merging; if it comes back red, that supersedes this review.
Recommended fixes — non-blocking: Issue 7 (Low, non-blocking getOutputId() still unused in production), Issue 8 (Low, consider rejecting duplicate plugin_output values explicitly in a follow-up), Issue 9 (Low, per-method Javadoc would still help).

Overall assessment: this is a genuine, verified resolution of the correctness gap that justifiably reopened my earlier "ready to merge" conclusion. Rather than patching around the two narrow bugs F1/F2/F4 exposed, the author addressed their structural root cause (duplicated, independently-drifting scheduler implementations) by extracting a single shared TransformDependencyScheduler, and backed every fix with tests that assert real DAG wiring and schemas, not just absence of an exception — including a new client-to-master E2E test that reproduces the original hang shape under a wall-clock timeout. I found no new correctness gap in my independent trace of this commit. The only remaining open item is CI completion, which I could not confirm at review time.

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

goutamadwant · 2026-09-19T17:32:49Z

Synced dev's merged CI fixes; all 83 focused graph/parser/CLI tests pass on Java 11. The graph patch is unchanged. Savepoint expiry and checkpoint-scheduler cleanup remain unresolved investigations, not fixes claimed by this sync. Fresh CI is queued.

SEZ9 · 2026-09-20T03:00:14Z

Thanks for the sync update, @goutamadwant. Since the graph patch itself is unchanged, the previously raised points on DryRunConnectValidator are still open. To move this forward, could you address (or push back on) each of the following:

PR12079-F1 (duplicate output IDs): When several transforms omit plugin_output and all fall back to the same default ID, the current scheduler double-decrements unresolvedInputCount and can release a multi-input dependent before all of its real inputs are scheduled, ending in a spurious "missing scheduled inputs" failure on a resolvable DAG. Please either key the ready-scheduler by unique output IDs (and reject/warn on collisions), or add a test showing this case is handled.
PR12079-F2 / PR12079-F4 (compatibility of the terminal fallback): Narrowing the legacy findLast fallback to single-transform jobs means multi-transform jobs whose last transform omits plugin_input, which previously worked via the implicit last-schema fallback, now hard-fail with ConfigCheckException. If this tightening is intentional, please say so explicitly and cover it in PR12079-F7; otherwise please keep the fallback for that case.
PR12079-F3 (getTransformInputIds vs getInputIds): Scheduling and validation should resolve input IDs through the same helper so a divergence can't surface as a confusing hard failure. Please consolidate on one.
PR12079-F6 (single-transform self-cycle): A single transform that references its own output currently passes dry-run via the legacy fallback, which contradicts the cycle-detection goal and diverges from the runtime parser. Please make the self-cycle case fail as well and add a test for it.
PR12079-F5 (duplicated ready-scheduler): Sharing the indexed scheduler between DryRunConnectValidator and MultipleTableJobConfigParser would prevent drift between dry-run and actual Zeta parsing. If you'd prefer to defer that to a follow-up, that's fine — just note it in the PR description.
PR12079-F7 (docs): The fail-fast rejection of cyclic/unresolved transform graphs is a user-visible change; please add a short docs/changelog/upgrade note.
PR12079-F8 (helper duplication / unused getOutputId()): Please reuse the existing input/output-ID resolution instead of re-parsing ReadonlyConfig, and remove getOutputId() if it has no production caller.

On the test claim: the 83 focused tests passing is good to hear, but none of the items above appear to be covered by them as far as I can tell from the thread — if some are, please point to the specific test names. Also, since savepoint expiry and checkpoint-scheduler cleanup are outside this PR's scope and still under investigation, please keep them out of this change so we can review the graph fix on its own. I'll take another look once the fresh CI run and the updates above land.

goutamadwant · 2026-09-20T06:03:15Z

Rechecked F1-F8 against the current head. F1's omitted-output case is already covered by testConnectDryRunDuplicateUnnamedOutputsWaitForOtherInput and testDuplicateUnnamedOutputsWaitForOtherJoinInput; the keyed waiting list is removed after its first release. F2/F4 cover terminal omitted-input fallback and default-first binding. F3/F5 use the same scheduler/input parser in runtime and dry-run. F6 rejects explicit self-cycles while preserving implicit legacy binding; F7 has EN/ZH compatibility notes.

For F8, getOutputId still has test callers; I propose leaving that optional API cleanup separate. No additional scheduler rewrite is needed for these cases. This source/test mapping is not a claim that unrelated CI failures are resolved.

SEZ9 · 2026-09-21T03:18:18Z

Thanks for the F1–F8 recheck, @goutamadwant — the mapping is helpful. A few follow-ups so I can close these out:

F1: Good that testConnectDryRunDuplicateUnnamedOutputsWaitForOtherInput and testDuplicateUnnamedOutputsWaitForOtherJoinInput cover the omitted-output case. Could you point me to where in DryRunConnectValidator the keyed waiting list is removed after its first release, and confirm that a second transform emitting the same default ID no longer decrements the dependent's unresolved input count?
F2 / F4: Please name the test(s) that cover a multi-transform job whose last transform omits plugin_input and previously relied on the implicit last-schema fallback, so I can confirm it still passes dry-run rather than hitting ConfigCheckException.
F3 / F5: To confirm — is the ready-scheduler and input parser now a single shared implementation used by both dry-run and MultipleTableJobConfigParser, or two copies kept in sync? If shared, where does it live? If separate, I'd still like them unified before merge.
F6: Rejecting explicit self-cycles while keeping implicit legacy binding sounds like a reasonable balance. Could you point me to the code and test that exercise the explicit self-cycle rejection so I can mark this resolved?
F7: Please point me to the EN/ZH compatibility notes in the diff (and link them from the PR description) so I can confirm the docs finding is addressed.
F8: Agreed to defer the getOutputId cleanup since it has test callers. Please open a follow-up issue and reference it here so it isn't lost.

Understood that this mapping doesn't claim anything about the unrelated CI failures. Once the items above are answered I'll do a final pass.

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

…ycle Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

DanielLeens

Hi @goutamadwant, thanks for the latest sync with dev. The head moved to ad8cadd12, so per our process I re-read the full diff on the new head rather than only the merge delta. Short version: the PR's own diff is unchanged, my earlier conclusion stands, and I have no new source-side concern.

What Problem Does This PR Solve?

User pain point: a cyclic transform graph (including an explicit self-reference) could hang job submission or --dry-run connect, and a multi-input transform could be created from only the inputs that happened to be available, silently dropping the remaining upstreams.
Fix approach: one shared TransformDependencyScheduler used by the runtime parser, the graph pre-check and the dry-run validator, with fail-fast errors for unresolved or cyclic graphs.
One-sentence summary: transform dependency resolution becomes one deterministic, fail-fast algorithm instead of two retry queues that could hang or mis-bind inputs.

1. Code Change Review

1.1 Core Logic Analysis

What changed since my last review (7bb568104). Only a merge of dev (merge commit ad8cadd12 plus the dev commits it brings in). I diffed all 12 non-doc PR-owned files between 7bb568104 and ad8cadd12 and the diff is empty. The only PR-owned files that differ are the two incompatible-changes.md files, and those differences are just other entries that landed on dev (the Java 11 runtime note and the Doris timestamp-timezone note); the "Transform Dependency Resolution" entry is still present in both EN and ZH. The PR's diff against current dev is still the same 14 files (+1598/-81) and the branch is 0 commits behind dev with no conflicts.
Runtime path (unchanged, re-verified). MultipleTableJobConfigParser.parse -> ConfigParserUtil.checkGraph -> TransformDependencyScheduler.scheduleTransforms, then parseTransform per scheduled transform (plugin discovery only after inputs exist). The dry-run path DryRunConnectValidator.validateTransforms -> scheduleTransforms calls the same scheduler and translates JobDefineCheckException into ConfigCheckException. Duplicate default outputs release a waiting join exactly once, the omitted-input fallback stays distinct from an explicit plugin_input, and an explicit self-reference ends in a concrete cycle error instead of looping.

1.2 Compatibility Impact

Partially incompatible, intentionally and documented. Valid graphs keep the same evaluation order and action indexes. Graphs that previously hung or silently mis-bound now fail fast at submission. The behavior change is documented in docs/en and docs/zh incompatible-changes.md with migration guidance. No config option, default or serialization format changes.

1.3 Performance / Side-Effect Analysis

Scheduling runs once per submission or dry-run in O((V + E) log V) and never touches the record path. This removes an unbounded-retry CPU risk rather than adding one.

1.4 Error Handling and Logging

Errors carry the unresolved output IDs, the available IDs and a plugin_input / plugin_output hint, or the concrete cycle path. Nothing is swallowed. No new issues.

2. Code Quality Assessment

Coding standards: the non-obvious parts of the scheduler are documented.
Test coverage and stability: ConfigParserUtilTest, MultipleTableJobConfigParserTest, SeaTunnelConfValidateCommandTest and JobExecutionIT#testRejectsExplicitTransformSelfCycleBeforeSubmission (bounded by assertTimeoutPreemptively) are deterministic and unchanged. Stability rating: Stable.
Documentation: EN and ZH are both updated and consistent.

3. Architectural Soundness

Long-term solution: one scheduler and three call sites, with no duplicated retry logic.

4. Issue Summary

No blocking issues found. No new issues from this pass.

5. Merge Recommendation

Conclusion: Ready to merge

Blockers: none on the source side.
Recommended fixes: none.

Existing threads. The open @SEZ9 request (assert the self -> self message in the dry-run self-cycle test) is already satisfied in SeaTunnelConfValidateCommandTest (testConnectDryRunRejectsSingleExplicitSelfCycle), and the F1 to F8 evidence in the earlier comments still applies line for line because the code is unchanged.

CI. On this head the Build check was still queued when I looked, so I cannot call it green yet. On the previous head (7bb568104) the red jobs were unrelated to this change (ElasticsearchIT slice tests that also fail on dev, fix open as #12711; the SplitClusterFaultToleranceIT worker-loss-during-cancel race, engine fix open as #12311; and an Azure Cosmos DB emulator connection failure). Neither fix is on dev yet, so please do not patch unrelated tests in this PR; if the new run shows the same failures, re-running only the failed jobs is the lowest-cost step. The remaining gates are CI and a write-capable maintainer.

Thanks again for the careful, patient work on this one.

SEZ9 · 2026-10-11T02:59:51Z

Thanks for syncing with dev. I re-read the full diff on the new head ad8cadd12 rather than only the merge delta. The PR-owned source files are unchanged between 7bb568104 and ad8cadd12; the only differences are in the two incompatible-changes.md files, and those are just other entries that landed on dev, with the "Transform Dependency Resolution" entry still present in both EN and ZH.

On this head the earlier points still hold as resolved: the runtime parser, the graph pre-check and the dry-run validator share one TransformDependencyScheduler; duplicate default outputs release a waiting join exactly once; an explicit self-reference ends in a concrete cycle error instead of looping; and the compatibility change is intentional and documented in the incompatible-changes notes.

My earlier conclusion stands and I have no new source-side concern from this sync.

DanielLeens · 2026-10-13T00:28:35Z

Quick CI update for @goutamadwant. In my last review (head ad8cadd12) the Build check was still queued, so I could not call it either way. It has now finished with failure (80 jobs passed, 3 failed, 11 skipped; fork run https://github.com/goutamadwant/seatunnel/actions/runs/38093119946). None of the 3 failures touch this diff, and everything in this PR's own area is green: unit-test on Java 11/17 (ubuntu and windows), engine-v2-it on Java 11/17, and transform-v2-it-part-1 / part-2 on Java 11/17.

The failed jobs:

elasticsearch-connector-it (11 and 17): ElasticsearchIT testElasticsearchWithPITSlice / testElasticsearchWithScrollSlice ("iterable lengths differ", for example expected 11 but was 0). This is the index-visibility race that [Test][Connector-V2][Elasticsearch] Poll for index visibility in ElasticsearchIT slice tests #12711 fixed, and [Test][Connector-V2][Elasticsearch] Poll for index visibility in ElasticsearchIT slice tests #12711 merged to dev on 2026-10-11, after this branch's last sync.
all-connectors-it-5 (11): OceanBaseCDCCompatibilityIT.testOceanBaseCdcWrapperRuntimeE2e errored with a ParameterResolution exception during container setup. This is an OceanBase CDC test in a different connector, unrelated to the transform dependency scheduler.

Suggested next step: merge the latest dev into this branch (the ES fix is now in that range; it is about 20 commits behind), then re-run only the failed jobs from the fork's Actions page ("Re-run failed jobs") rather than the whole workflow. Please do not patch unrelated tests in this PR.

My source-side conclusion is unchanged: no source blockers, ready to merge once CI is green and a write-capable committer approves.

SEZ9 · 2026-10-13T03:04:27Z

Thanks for the CI breakdown on ad8cadd12 (run 38093119946). Agreed that the three failures look unrelated to this change, and the jobs covering this PR's own area (unit-test, engine-v2-it, transform-v2-it-part-1/part-2) are green.

The suggested path sounds right to me: merge the latest dev into this branch so the Elasticsearch fix is in range, then re-run only the failed jobs. Please don't patch unrelated tests in this PR.

Once CI is green after the dev sync, I'm happy to take another look.

…ycle

DanielLeens

Re-reviewed at current head 612b5ede51 (verified immediately before posting).

I traced both runtime parsing and connect dry-run through the shared TransformDependencyScheduler. It records every distinct missing input before plugin discovery, releases each waiting set only once when its output becomes available, and preserves the legacy retry queue ordering/action indices through the Fenwick count. It also keeps the two deliberate legacy fallbacks narrowly scoped: a final omitted/explicit-empty input and the single-transform mismatch. The remaining graph now fails deterministically with either the concrete cycle path or unresolved input IDs; it can no longer construct a multi-input transform from a partial input set.

The core and dry-run paths are covered for cycles, unresolved/mixed inputs, reverse declaration, duplicate IDs, explicit-empty/single-transform compatibility, and index boundaries. I found no source-level blocker.

Merge gate: required Build is currently FAILURE. Please resolve or rerun it successfully for this exact head before merging.

Conclusion: code is ready for maintainer approval after a green Build. COMMENT review only because my reviewer account is read-only.

SEZ9 · 2026-10-19T03:19:29Z

Thanks for the follow-up work on this. Based on the re-review at 612b5ede51, here is where I see the previous points standing:

Looks addressed (pending my own pass over the head):

F1 (duplicate output IDs double-decrementing) and F5 (scheduler duplicated between dry-run and MultipleTableJobConfigParser): the shared TransformDependencyScheduler releasing each waiting set only once, with both runtime parsing and connect dry-run going through it, is exactly the shape I was hoping for.
F2 / F4 / F6 (legacy fallback scope): narrowing the fallbacks to "final omitted/explicit-empty input" and "single-transform mismatch" is a reasonable compromise. Please confirm in the PR description which previously-accepted multi-transform configs now fail with ConfigCheckException, so we have that recorded for users.

Still open — please reply or push:

F3: does the dry-run path still resolve inputs via getInputIds for validation while the scheduler uses getTransformInputIds? If both now go through a single resolution helper, point me to it; if not, please unify them so a divergence can't surface as a confusing "missing scheduled inputs" failure.
F7: I don't see any docs/changelog/upgrade-note change mentioned. Since cyclic or unresolved transform graphs now fail fast, please add a short note describing the new behavior and the retained fallbacks.
F8: please confirm whether getOutputId() is now used in production code, or remove it, and drop any remaining helpers that re-parse ReadonlyConfig where the existing resolution already exists.
The required Build check is reported as FAILURE on this head. Please get it green (or share the failure if it looks unrelated) before I do the final approval pass.

Once those are in, I'll do a last read of the head and approve.

DanielLeens · 2026-10-19T12:02:14Z

Thanks @SEZ9 for the careful follow-up. I re-checked each open item against the current head 612b5ede517b5f7b800c2695c8e2f3e88a789b97 (the head is unchanged since my last review), and I believe all four are already closed on this head. Details with locations so you can verify directly:

1. F3 (single resolution path). Transforms resolve their IDs exactly once, in the ScheduledTransform constructor: TransformDependencyScheduler.java:235 calls ConfigParserUtil.getInputIds(readonlyConfig), and exposes the results via getOutputId() (:247) and getInputIds() (:251). DryRunConnectValidator.validateTransform reads scheduledTransform.getOutputId() / getInputIds() (:240, :242) instead of re-parsing the config. There is no getTransformInputIds / getTransformOutputId helper left. The one remaining getInputIds(readonlyConfig) call in DryRunConnectValidator (:305) is in the sink validation path, which has no scheduler involvement, so it cannot diverge from transform scheduling.

2. F7 (docs). Present in both languages: docs/en/introduction/concepts/incompatible-changes.md:104 (### Transform Dependency Resolution) and docs/zh/introduction/concepts/incompatible-changes.md:75 (the matching Transform dependency entry). They describe the fail-fast behavior (JobDefineCheckException at Zeta parse time, ConfigCheckException in connect dry-run), including explicit self-references in single-transform jobs. Your request to record in the PR description which previously accepted multi-transform configs now fail is also covered: the PR body states that the old resolver could loop indefinitely on cyclic graphs and could build a multi-input transform from only the inputs already available, silently dropping the remaining upstreams.

3. F8. getOutputId() on ScheduledTransform has production callers (MultipleTableJobConfigParser and DryRunConnectValidator.validateTransform), and the static getOutputId helper no longer exists in ConfigParserUtil.

4. Build. The required Build check is now SUCCESS for this head (completed 2026-10-19T05:22Z, fork run https://github.com/goutamadwant/seatunnel/actions/runs/39008501172). This is a statement about that specific run on 612b5ede5, not about any later head.

On my side there is no remaining source-side blocker, and I have not run anything locally. From here it only needs your final read and a write-capable committer to merge. Thanks again to @goutamadwant for the patient iteration on this one.

DanielLeens

Thank you for syncing dev again, @goutamadwant. This is a full re-review of the entire current diff at head b190936b and of the runtime call chain, not only of the merge. The PR-owned source and test files are byte-identical to the previous head 612b5ede; the only PR files that differ are the two incompatible-changes.md files, which picked up unrelated entries from dev (checked with a compare between the two heads).

What Problem Does This PR Solve?

User pain point: In Zeta job parsing and --dry-run connect, a cyclic transform graph made the retry queue spin forever, and a multi-input transform was built as soon as any one of its configured inputs existed, silently dropping the other upstream edges (#12077).
Fix approach: One shared TransformDependencyScheduler replaces both retry queues. It indexes waiting transforms per missing input, releases each set exactly once, preserves the legacy evaluation order and action indexes with a Fenwick tree, detects cycles with Kahn's algorithm, and delays plugin discovery until all inputs exist.
One-sentence summary: Cycles and unresolved inputs now fail fast with a concrete message, multi-input transforms always keep all configured upstreams, and valid graphs keep their order, names and indexes.

1. Code Change Review

1.1 Core Logic Analysis

Before (MultipleTableJobConfigParser.parseTransform, simplified):

inputs = inputIds.stream().map(tableWithActionMap::get).filter(Objects::nonNull)...;
if (inputs.isEmpty()) { if (transforms.isEmpty()) { inputs = findLast(...); } else { transforms.offer(config); return; } }

After (MultipleTableJobConfigParser.java:468-524, TransformDependencyScheduler.java:53-151):

List<ScheduledTransform> scheduled = scheduleTransforms(transformConfigs, tableWithActionMap.keySet());
// each transform is parsed only after every distinct plugin_input exists; plugin jars are resolved afterwards (:520)

Runtime path:

Client: ClientJobExecutionEnvironment.getLogicalDag -> MultipleTableJobConfigParser.parse
  -> ConfigParserUtil.checkGraph (simple graph :72-78 / complex graph phase 6 :175-180)
  -> parseSource -> parseTransforms -> TransformDependencyScheduler.scheduleTransforms (:471) -> parseTransform per scheduled item
CLI: DryRunConnectValidator.validateTransforms (:215) -> same scheduler -> validateTransform

Key findings (verified on this head):

TransformDependencyScheduler.java:116-124: dependents wait in waitingByInputId and are released through remove(outputId), so each ID satisfies a dependency exactly once even when several transforms produce the same output ID (the earlier double-decrement concern is closed by this).
:127-150: after the ready loop, remaining transforms go first through checkTransformCycles (explicit self-edges and real cycles throw, the omitted-input implicit chain is exempt at :292), then through the two documented fallbacks (a single remaining omitted/empty-input transform, or a one-transform job where no configured input exists). Everything else fails with the unresolved IDs listed.
The Fenwick bookkeeping (:166-199) reproduces the old poll-derived action index, so default action names stay Transform[n]-factory. As an independent cross-check I modelled the retry queue (all-inputs readiness) and this scheduler's ready/queue-head/Fenwick logic in a small script and compared order and action index on 200,000 random DAGs, including reverse declaration order and duplicate output IDs: no difference, and cyclic cases were rejected by both. That is my own model of the algorithm, not the repository code, so I treat it as supporting evidence only.
Plugin discovery moved after readiness (MultipleTableJobConfigParser.java:520), so a transform that is not ready no longer triggers repeated getTransformPluginJarPaths lookups.
ConfigParserUtil.java:72-78 and :175-180 run the scheduler during graph checking, so a cycle is rejected before any DAG is built on the client; the ID set passed is the set of source output IDs, which matches tableWithActionMap at the start of parseTransforms.

1.2 Compatibility Impact

Partially incompatible, intentionally and documented (docs/en/introduction/concepts/incompatible-changes.md, "Transform Dependency Resolution", with the zh counterpart):

Valid graphs where every plugin_input resolves keep their evaluation order, action indexes, default names and ordering of inputs; duplicate input IDs behave as before.
Configs that relied on the old bugs now fail at submission: a multi-input transform with an input ID that never appears (typo, previously silently dropped), mutually dependent transforms (previously an endless loop), and several unresolved omitted-input transforms (previously an endless loop).
The two legacy fallbacks are retained and narrowly scoped, so older single-transform configs with a mismatched ID, and a final omitted-input transform, still work.
No option, API or wire change, and no persisted-state change: the logical DAG is produced at submission and serialized with the job, so running jobs and master failover are not re-parsed. A resubmission, including a restore that re-submits the same config, is parsed again (see issue 2).

1.3 Performance / Side-Effect Analysis

Parsing cost drops from a repeated retry loop (unbounded for cycles, quadratic polling for reverse-ordered chains, with plugin discovery per poll) to O((V + E) log V) time and O(V + E) memory, executed once per parse and once more in graph checking. V is the number of transforms in a job, so this is negligible at any realistic job size, and nothing here runs per record, so the production data volume does not matter for this change. No new threads, locks or retries; no secrets are logged (the error message lists output/input IDs and available IDs only).

1.4 Error Handling and Logging

Failures raise JobDefineCheckException on the Zeta path and are re-wrapped as ConfigCheckException in dry-run (DryRunConnectValidator.java:224-231), both with the cycle path or the unresolved output <- inputs pairs. Issues:

Low - CI is red for reasons outside this diff, and it did not execute this PR's own unit tests on this head. The four unit-test legs fail in connector-jdbc at JdbcOptionRuleCompletenessTest.testSinkRuleDeclaresAllSinkOptions (unknown option numeric_special_value_as_null), the known dev break tracked in #12874 with the fix in #12875 (still open). Because the reactor stops there, Engine : Core, Engine : Client, Core : Starter and Engine : Server show as SKIPPED in those jobs. jdbc-connectors-it-part-1 (17) failed while resolving the os-maven-plugin build extension (an artifact-resolution problem), and the two mysql-cdc jobs were cancelled. The jobs that cover this change did pass on this head: engine-v2-it (11 and 17, including the new JobExecutionIT#testRejectsExplicitTransformSelfCycleBeforeSubmission), transform-v2-it-part-1/2, engine-k8s-it and engine-resource-managers-it. The previous head had a green unit-test run, and the PR-owned files have not changed since. Best improvement: once #12875 is merged, sync dev and rerun only the failed jobs so the unit-test legs reach the engine and starter modules on the final head. Raised by another reviewer: no.
Low - Document the savepoint and resubmission implication (new on my side). Location: the "Transform Dependency Resolution" entry in incompatible-changes.md. A job that is resubmitted from a savepoint with a config that contains an unresolved or partial plugin_input will now fail at parse until the config is corrected, and once corrected the previously dropped upstream feeds the transform, which changes the data a stateful transform sees. This is the intended direction, but a sentence in the migration guide would save operators a surprise. Sorry for not raising this in earlier rounds. Raised by another reviewer: no.
Low - Comment the fallback decision (new on my side). Location: TransformDependencyScheduler.java:136-149. The emptyInputFallback || singleTransformLegacyFallback branch is the single place where an otherwise ambiguous graph is accepted; a short comment saying why anyInputAvailable is part of the single-transform rule (a one-transform job with some existing input is still treated as a real mismatch) would make the contract obvious to the next reader. Sorry for not raising this earlier. Raised by another reviewer: no.

2. Code Quality Assessment

2.1 Coding Standards

Good: the class and the main method carry Javadoc stating complexity and fallback scope, the Fenwick section is commented with its purpose, and the nested ScheduledTransform hides its state behind getters. ConfigParserUtil.getInputIds became public only so that the starter module reuses the same input resolution (no divergent re-parse remains, which closes the earlier single-resolution-path question); the only other getInputIds use in DryRunConnectValidator is on the sink path.

2.2 Test Coverage and Test Stability

Strong: ConfigParserUtilTest, MultipleTableJobConfigParserTest, SeaTunnelConfValidateCommandTest and a new JobExecutionIT case cover two-node and self cycles, partially and wholly unresolved inputs, mixed source/transform inputs, duplicate input IDs and duplicate unnamed outputs, valid reverse-declared chains, terminal omitted-input binding, explicit-empty-input fallback, action-index overflow, and both the runtime and the dry-run entry points. Hang regressions use assertTimeoutPreemptively, so a returning infinite loop turns the test red instead of hanging CI. The only deletions in test code are a relocated mockCatalogTable helper and a replaced OptionRule line in a test factory that gains an optional expected_table, so no assertion was weakened. Stability rating: Stable (no Thread.sleep; timeouts are generous at 30 seconds against millisecond work).

2.3 Documentation Updates

en and zh incompatible-changes.md are both updated with behavior, affected components and migration guidance. See issue 2 for the one missing sentence.

3. Architectural Soundness

3.1 Elegance

Long-term solution: one scheduler in engine-core shared by the runtime parser, the graph pre-check and the dry-run validator, instead of three divergent retry loops.

3.2 Maintainability

Much better than before; the Fenwick tree is the one non-obvious piece and it is documented and pinned by the index-boundary tests.

3.3 Extensibility

Adding a new fallback or dependency source means touching one class.

3.4 Historical-Version Compatibility

Only parsing at submission changes. No serialized job, checkpoint or savepoint format is touched, so there is no upgrade-path impact beyond the documented stricter validation of previously ambiguous configs.

4. Issue Summary

Number	Issue	Location	Severity
1	Build is red from the pre-existing JDBC break; unit-test legs never reach engine-core, client and starter on this head	fork `Build` run, `connector-jdbc` `JdbcOptionRuleCompletenessTest`	Low (merge gate)
2	Migration guide should mention savepoint resubmission with unresolved or partial inputs	`docs/*/introduction/concepts/incompatible-changes.md`	Low
3	Comment why the single-transform fallback depends on `anyInputAvailable`	`TransformDependencyScheduler.java:136-149`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers:

A green Build on the final head (sync dev after #12875 lands and rerun the failed jobs). I have no source-level blocker.

Recommended fixes (non-blocking):

Add the savepoint/resubmission sentence to the migration guide (issue 2).
Add the short fallback comment (issue 3).

Overall assessment: A careful, well-tested fix for a real hang and a silent data-lineage bug, with the legacy order and names preserved for valid graphs. The change is exercised on every job submission, but its blast radius is limited to parse-time validation and it is covered by unit and end-to-end tests.

Better alternative: None that is simpler without reintroducing the retry loop; keeping one shared scheduler is the right shape.

SEZ9 · 2026-10-20T03:45:49Z

Thanks for the detailed re-check against head 612b5ede517b5f7b800c2695c8e2f3e88a789b97, and for noting that the Build check is reported as SUCCESS for run 39008501172 on 612b5ede5.

The locations you listed for F3 (single resolution path in the ScheduledTransform constructor), F7 (the Transform Dependency Resolution entries in both the en and zh incompatible-changes docs) and F8 (production callers of getOutputId() and removal of the static helper) are exactly what I needed. I'll verify them directly against the diff on this head rather than re-requesting anything, and will mark the three items resolved once that matches up.

No further questions from my side for now; I'll do the final read-through on 612b5ede5 and follow up here.

DanielLeens · 2026-10-22T10:24:07Z

Thanks @SEZ9. The earlier green run you cited is for 612b5ede5, whereas the current head is b190936bbbcb82f4758aeff6b7f565f44d16739a. Its Build run 39254498031 is failed, so the older success cannot serve as the current-head gate. The inspected unit log fails JdbcOptionRuleCompletenessTest.testSinkRuleDeclaresAllSinkOptions on numeric_special_value_as_null; jdbc-connectors-it-part-1 (17) also stops before tests because Maven cannot resolve the os-maven-plugin extension dependencies, including commons-lang3:3.5, from Central. That is a bootstrap/dependency-resolution signal, for which a targeted retry after repository availability recovers is more informative than changing this parser fix. Fix #12875 is still open and current dev retains the fixture mismatch, so syncing alone is not yet a cure for that unit failure.

My previous source-level conclusion remains clear, with the migration/comment notes non-blocking; your direct verification remains yours to close. Separately, GitHub currently reports real conflicts against dev. Please resolve those conflicts, and once the shared JDBC correction is available, include it and rerun the affected checks on the resulting head. I will re-review the full updated diff after a new commit. This follow-up does not repeat the earlier source review or declare current CI green.

Current status evidence: head b190936bbbcb82f4758aeff6b7f565f44d16739a, base dev, compare diverged (ahead 17, behind 18), mergeable state dirty.

CI references: current Build check, Run / unit-test (11, ubuntu-latest), Run / jdbc-connectors-it-part-1 (17, ubuntu-latest).

…ycle

DanielLeens

Thanks @goutamadwant for keeping this branch in sync with dev again. The head moved to 3696a4b3c07d after my last review at b190936bbbcb, so this is a fresh full re-read of the PR rather than a look at the merge delta only. Analysis is static; nothing was built or run locally.

What Problem Does This PR Solve?

User pain point: In Zeta job parsing and --dry-run connect, a cyclic transform graph made the retry queue spin forever, and a multi-input transform was built as soon as any one of its configured inputs existed, silently dropping the other upstream edges (#12077).
Fix approach: one shared TransformDependencyScheduler in engine-core replaces the retry queues in the runtime parser, the graph pre-check and the dry-run validator. It releases each waiting set exactly once, keeps the legacy evaluation order and action indexes, detects cycles, and delays plugin discovery until every input exists.
One-sentence summary: cycles and unresolved inputs now fail fast with a concrete message, and valid graphs keep their order, names and indexes.

1. Code Change Review

1.1 Core Logic Analysis

What changed since b190936bbbcb: I diffed every PR-owned source, test and E2E file between the two heads and they are byte-identical. The only PR file that differs is docs/en/introduction/concepts/incompatible-changes.md, which gained other teams' entries from dev (Markdown child_ids, ARRAY_MAX/MIN, DataValidator LENGTH, IMap WAL). The "Transform Dependency Resolution" entry (incompatible-changes.md:179) is unchanged. So my earlier analysis of the code still holds on this head:

Client: MultipleTableJobConfigParser.parse
  -> ConfigParserUtil.checkGraph (cycle / unresolved pre-check)
  -> parseTransforms -> TransformDependencyScheduler.scheduleTransforms -> parseTransform (plugin discovery only after readiness)
CLI:    DryRunConnectValidator.validateTransforms -> same scheduler -> validateTransform

Each output ID releases its waiters once, so duplicate output IDs cannot release a multi-input transform early.
Only after the ready loop does it check cycles, then apply the two documented fallbacks (TransformDependencyScheduler.java:136-149), then fail with the unresolved output <- inputs pairs.
Dry-run and runtime share one resolution path, so they cannot disagree.

1.2 Compatibility Impact

Partially incompatible by design and documented in en and zh: configs that relied on the old hang or on silently dropped inputs now fail at submission. Valid graphs keep their order and action names. No option, API, wire or persisted-state change, so running jobs and master failover do not re-parse anything.

1.3 Performance / Side-Effect Analysis

Parse-time only, O((V + E) log V) with V as the number of transforms, nothing per record. No new threads, locks or retries. Nothing sensitive is logged.

1.4 Error Handling and Logging

JobDefineCheckException on the Zeta path and ConfigCheckException in dry-run, both with the cycle path or the unresolved IDs. No new findings.

2. Code Quality Assessment

2.1 Coding Standards

Good. Javadoc and comments on the scheduler are present.

2.2 Test Coverage and Test Stability

Strong and unchanged: ConfigParserUtilTest, MultipleTableJobConfigParserTest, SeaTunnelConfValidateCommandTest and JobExecutionIT#testRejectsExplicitTransformSelfCycleBeforeSubmission cover cycles, partial or wholly unresolved inputs, duplicate IDs, reverse-declared chains, fallbacks and index overflow. Hang regressions use assertTimeoutPreemptively. Stability rating: Stable.

CI for this head: the apache-side PR signal is success, and it is only a pointer. The fork Build run for 3696a4b3c07d (https://github.com/goutamadwant/seatunnel/actions/runs/39903819357) is still queued, so there is no result for this head yet. On the previous head the red unit-test legs came from JdbcOptionRuleCompletenessTest (numeric_special_value_as_null) in connector-jdbc, which stopped the reactor before engine-core, client and starter ran. This head contains #12800, and the test in the tree now references NUMERIC_SPECIAL_VALUE_AS_NULL explicitly, so I expect that break to be gone, but that is a reading of the code, not an observed run.

2.3 Documentation Updates

en and zh entries exist. See Issue 1 below for the one remaining sentence.

3. Architectural Soundness

3.1 Elegance

Long-term solution: one shared scheduler instead of three divergent loops.

3.2 Maintainability

Much better than before. The Fenwick bookkeeping is documented and pinned by index-boundary tests.

3.3 Extensibility

New fallbacks or dependency sources touch one class.

3.4 Historical-Version Compatibility

Only submission-time parsing changes. No serialized job, checkpoint or savepoint format is touched.

4. Issue Summary

Carried over from my previous review, both still open at this head and both non-blocking:

Number	Issue	Location	Severity
1	Migration guide does not mention that a job resubmitted from a savepoint with an unresolved or partial `plugin_input` now fails at parse, and once fixed the previously dropped upstream feeds the transform	`docs/en` and `docs/zh` `incompatible-changes.md`, "Transform Dependency Resolution"	Low
2	No comment explains why the single-transform fallback depends on `anyInputAvailable` (a one-transform job with some existing input is still treated as a real mismatch)	`TransformDependencyScheduler.java:136-149`	Low

5. Merge Recommendation

Conclusion: Ready to merge

Blockers: none in source. The only remaining gate is a green Build on this head. Please let the fork run above finish and share the result; if only unrelated jobs are red, rerun just those. Please do not add unrelated patches to this PR.

Recommended non-blocking: Issues 1 and 2 above (one sentence in each migration guide, one short code comment).

Overall assessment: a careful, well-tested fix for a real hang and a silent data-lineage bug. The PR-owned code has not changed across the recent dev syncs. I have no source-level objection; I am holding the approval only because there is no CI result for this head yet. A maintainer with write access still has to do the final approval and merge.

SEZ9 · 2026-10-24T03:32:41Z

Thanks for the rebase. Comparing b190936bbbcb with 3696a4b3c07d, the PR-owned source, test and E2E files are unchanged and only the incompatible-changes doc picked up unrelated entries from dev, so here is where my earlier points stand on this head.

Addressed

F1: each output ID releases its waiters exactly once, so duplicate output IDs can no longer release a multi-input transform early.
F3 / F5: dry-run and runtime now share the single TransformDependencyScheduler, so the two paths cannot diverge.
F6: cycle detection runs after the ready loop and before the fallbacks are applied, so a self-cycle no longer slips through the legacy fallback.
F7: the behavior change is documented in the en and zh incompatible-changes entries.
F2 / F4: the narrowed fallback is an intentional, documented incompatibility, and the two remaining fallbacks are spelled out in the scheduler, so I consider this settled.

One remaining question

F8: is the unused getOutputId() accessor still present, and has the input/output ID resolution been consolidated onto the existing helpers instead of re-parsing ReadonlyConfig? A short note either way is fine.

Otherwise this looks good to me.

DanielLeens · 2026-10-24T10:48:50Z

Thanks @SEZ9 for the careful re-check on the synced head. On F8, I re-read head 3696a4b3c07d28fd0903174cac4968e561c02b67 (static read only, nothing run locally), and I also need to correct one detail in my earlier F8 note. Sorry for the imprecision there.

F8 on this head:

getOutputId() is still present and has a production caller, so it is not unused. ScheduledTransform.getOutputId() (TransformDependencyScheduler.java:247) is read by DryRunConnectValidator.validateTransform (DryRunConnectValidator.java:240), next to getInputIds() (:242). Tests also use it (ConfigParserUtilTest, SeaTunnelConfValidateCommandTest).
My correction: I said earlier that MultipleTableJobConfigParser also calls getOutputId(). It does not. parseTransforms (MultipleTableJobConfigParser.java:470-480) passes only getActionIndex(), getConfig() and isLegacyFallback() into parseTransform, which then derives the IDs from ReadonlyConfig again: getInputIds(readonlyConfig) at :492 and PLUGIN_OUTPUT ... orElse(DEFAULT_ID) at :507 / :522.
Consolidation: input IDs are resolved through the shared ConfigParserUtil.getInputIds helper in all three places: the scheduler constructor (TransformDependencyScheduler.java:235, now public for this reason), the runtime parser (:492) and dry-run. The old static output-id helper is gone. The output ID uses the same getOptional(PLUGIN_OUTPUT).orElse(DEFAULT_ID) expression everywhere, because ConfigParserUtil has no output-id helper. All paths compute IDs the same deterministic way, so the scheduler and the runtime parser cannot disagree. The second read in the runtime path is redundant, but it is not a correctness issue.

Optional cleanup for @goutamadwant (not blocking): pass scheduledTransform.getInputIds() / getOutputId() into parseTransform instead of re-reading the config. The runtime path then reads IDs exactly once, as dry-run already does. A short note that the current form is intentional is equally fine.

For completeness: the Build check on 3696a4b3c07d is currently reported as FAILURE, so the current-head CI gate is still open. My source-level conclusion from the latest review is unchanged.

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens

Thanks @goutamadwant, and thank you for picking up both of my non-blocking items so quickly. The head moved to 9152a91fa278 after my last review at 3696a4b3c07d. It is one new commit ([Docs][Zeta] Explain restored transform-input migration) touching only the en and zh incompatible-changes.md and one comment in TransformDependencyScheduler.java. I still did a fresh full re-read of the diff and call chain at this head rather than only the delta. Analysis is static; nothing was built or run locally.

What Problem Does This PR Solve?

User pain point: in Zeta job parsing and --dry-run connect, a cyclic transform graph made the retry queue spin forever, and a multi-input transform was built as soon as any one of its configured inputs existed, silently dropping the other upstream edges (#12077).
Fix approach: one shared TransformDependencyScheduler in engine-core replaces the retry queues in the runtime parser, the graph pre-check and the dry-run validator. It releases each waiting set exactly once, keeps the legacy evaluation order and action indexes, detects cycles, and delays plugin discovery until every input exists.
One-sentence summary: cycles and unresolved inputs now fail fast with a concrete message, and valid graphs keep their order, names and indexes.

1. Code Change Review

1.1 Core Logic Analysis

What changed since 3696a4b3c07d (verified with the compare view): incompatible-changes.md (en, zh) and a 2-line comment above singleTransformLegacyFallback in TransformDependencyScheduler.java. No executable line changed, and all PR-owned source, test and E2E files are otherwise identical, so my earlier analysis of the code still holds:

Client: MultipleTableJobConfigParser.parse
  -> ConfigParserUtil.checkGraph (cycle / unresolved pre-check)
  -> parseTransforms -> TransformDependencyScheduler.scheduleTransforms -> parseTransform (plugin discovery only after readiness)
CLI:    DryRunConnectValidator.validateTransforms -> same scheduler -> validateTransform

Each output ID releases its waiters once, so duplicate output IDs cannot release a multi-input transform early.
After the ready loop it checks cycles, then applies the two documented fallbacks, then fails with the unresolved output <- inputs pairs.
Dry-run and runtime share one resolution path, so they cannot disagree.
This is the normal job-submission parse path (every job with transforms), not a recovery-only path.

1.2 Compatibility Impact

Partially incompatible by design, and documented in both languages: configs that relied on the old hang or on silently dropped inputs now fail at submission. The new migration text also states that the same validation runs when a job is resubmitted from a savepoint and that a corrected or reordered graph now feeds previously dropped upstreams to the transform, so users should review downstream output. That closes my earlier Issue 1. Valid graphs keep their order and action names. No option, API, wire or persisted-state change.

1.3 Performance / Side-Effect Analysis

Parse-time only, O((V + E) log V) in the number of transforms, nothing per record. No new threads, locks or retries.

1.4 Error Handling and Logging

JobDefineCheckException on the Zeta path and ConfigCheckException in dry-run, both with the cycle path or unresolved IDs. No new findings.

2. Code Quality Assessment

2.1 Coding Standards

Good. The new comment explains why a partially matched input list is a mismatch and not the single-transform fallback, which closes my earlier Issue 2.

2.2 Test Coverage and Test Stability

Strong and unchanged: ConfigParserUtilTest, MultipleTableJobConfigParserTest, SeaTunnelConfValidateCommandTest and JobExecutionIT#testRejectsExplicitTransformSelfCycleBeforeSubmission cover cycles, partial or wholly unresolved inputs, duplicate IDs, reverse-declared chains, fallbacks and index overflow. Hang regressions use assertTimeoutPreemptively. Stability rating: Stable.

2.3 Documentation Updates

en and zh entries exist and are in sync for this change.

3. Architectural Soundness

3.1 Long-term solution: one shared scheduler instead of three divergent loops.
3.2 Maintainability: much better than before; the Fenwick bookkeeping is documented and pinned by index-boundary tests.
3.3 Extensibility: new fallbacks or dependency sources touch one class.
3.4 Historical-version compatibility: only submission-time parsing changes; no serialized job, checkpoint or savepoint format is touched.

4. Issue Summary

No open code or documentation issues at this head. My two earlier Low items (migration guide wording, missing comment) are resolved.

Number	Issue	Location	Severity
-	None open	-	-

5. Merge Recommendation

Conclusion: Ready to merge

Blockers (must fix): none in source or docs.

Recommended fixes (non-blocking): none.

CI status of this head: the apache-side Build check (https://github.com/apache/seatunnel/runs/121071491816) is still pending and only points at the fork's run, so there is no green result for this head yet. My approval is held only on a green Build for 9152a91fa278; if a job fails, please share the failing link and I will help determine whether it is related to this diff, and then only that job should be rerun. The branch is 14 commits behind dev with no conflicts, so there is nothing to sync for correctness reasons.

Overall assessment: a careful, well-tested fix for a real hang and a silent data-lineage bug, with a clear migration note. I have no source-level objection. A maintainer with write access still has to do the final approval and merge.

DanielLeens

Thanks for the follow-up, @SEZ9 and @goutamadwant. I re-read the full fourteen-file diff and current call chains. The F1-F8 technical points are addressed as detailed below, and the two small items from my latest full review (savepoint migration wording and the partial-input fallback comment) are now fixed. Sorry for not calling out the new public resolver/wrapper documentation gap earlier: Issue 4 below is a carryover delivery omission, not a regression introduced by this documentation update.

What problem this PR solves

User pain: Transform configuration parsing can loop forever on unresolved cycles or construct a multi-input transform from only the already available inputs, silently dropping intended upstreams.
Fix approach: Use one indexed dependency scheduler in graph validation, runtime parsing and connect dry-run; wait for every distinct dependency, reject cycles/unresolved graphs, retain documented legacy fallbacks and preserve legacy action indexes.
One-line summary: Resolve valid transform graphs deterministically and fail invalid graphs before plugin discovery instead of hanging or dropping inputs.

Example: For a transform requiring [sourceA, transformB], sourceA being ready is no longer enough: transformB must also be available before the factory receives its schemas.

1. Code-change review

1.1 Core logic analysis

Fourteen files, +1604/-81, including scheduler/parser, dry-run, UT/E2E and EN/ZH migration notes. The complete target-relative diff was read at 9152a91fa278e7827e66b73c691e6f1e8f961abc, not just the latest commit. Submission-time production parsing plus matching test/documentation changes.

Before / after:

Before: poll queue; filter unavailable inputs; accept partial input list or requeue indefinitely
After:  scheduleTransforms -> missing distinct IDs -> ready set -> release waiters once
        -> unresolved cycle check -> narrow fallback -> contextual failure
Before: discovery could happen before readiness
After:  parseTransform checks readiness before getTransformPluginJarPaths

Complete relevant execution chain:

ClientJobExecutionEnvironment.getLogicalDag -> parser.parse
  -> ConfigParserUtil.checkGraph():70-78 / complex phase :175-181
     -> TransformDependencyScheduler.scheduleTransforms
  -> MultipleTableJobConfigParser.parseTransforms():470-480
     -> same scheduler -> scheduled actionIndex/config/fallback
     -> parseTransform():492-529 -> all inputs present -> plugin discovery
     -> transform factory -> produced tables/actions -> downstream sink
Connect dry-run -> DryRunConnectValidator.validateTransforms():211-225
  -> scheduleTransforms wrapper :228-235 -> same scheduler
  -> validateTransform :240-280 reads scheduled IDs -> schema/factory checks
Scheduler :59-83 missing ID index -> circular TreeSet ready selection
  -> Fenwick remaining counts preserve queue-poll indexes :87-119
  -> waitingByInputId.remove(outputId) :121-129 releases once per ID
  -> unresolved check :132-157; cycle check before fallback
  -> final omitted/empty input OR single-transform total mismatch fallback
  -> otherwise JobDefineCheckException (translated to ConfigCheckException in dry-run)
Running/checkpoint recovery uses the submitted DAG; resubmission from savepoint
  parses again and therefore applies the documented stricter validation

Key findings:

+1 to @SEZ9's F1 requirement: removing the waiting list on first output availability prevents a duplicate producer from decrementing a join twice. Tests assert actual scheduling order and upstream edges, not just a final success.
F2/F4/F6 are resolved: explicit self-cycles fail before fallback; omitted input first resolves DEFAULT_ID; only the documented terminal/one-transform exceptions remain.
F3/F5: both callers use the same scheduler and ConfigParserUtil.getInputIds. Dry-run reads scheduled IDs directly. Runtime re-reads immutable config with the same helper/expression; redundant, but not a divergent resolver.
F7: both migration notes now explicitly cover savepoint resubmission and previously dropped inputs becoming actual downstream inputs.
F8: ScheduledTransform.getOutputId has a production caller at DryRunConnectValidator:240. It is not unused. Runtime parseTransform does not call that accessor; the earlier correction to that distinction still stands.

Community context: All SEZ9 discussion findings and their later resolutions were read. I agree with the latest source assessment, and provide concrete F8 closure rather than repeating the request. There is no non-Daniel formal approval to treat as an effective final repository gate.

1.2 Compatibility impact

Partially incompatible, intentionally documented. Valid graphs retain evaluation order/default action names. Cyclic/unresolved/partially bound configurations that previously hung or silently discarded inputs now fail during parsing/dry-run. No config key/default, wire format or persisted state format changes; exposing the existing input resolver is an additive API change. Migration: correct explicit input/output references and cycles before submission/resubmission; check downstream output when an intended upstream was previously dropped. EN/ZH incompatible-changes entries describe the retained fallbacks.

1.3 Performance / side effects

Parse-time CPU/memory changes only; no per-record cost, new runtime thread, lock, retry or resource lifecycle. Indexed missing dependencies plus TreeSet/Fenwick bookkeeping avoids replaying unsuccessful queue polls: O((V+E) log V) scheduling with O(V+E) bookkeeping. Graph pre-check and parser can invoke the scheduler separately, a bounded repeated parse-time cost. Long action indexes are range-checked before narrowing to int.

1.4 Error handling and logging

Unresolved/cycle failures identify output/input IDs and concrete cycles before plugin discovery. Dry-run translates the exception type while retaining the cause/message. Unknown upstream schemas remain explicitly skipped, not falsely validated. No credential values are introduced into the new messages.

Issue 3: Optionally pass scheduled IDs into the runtime parser

Location: MultipleTableJobConfigParser.java:470-507.
Problem: Runtime parseTransform reads the same immutable config IDs again, while dry-run uses ScheduledTransform accessors. This is the redundancy discussed under F8, not a correctness divergence.
Risk: Small maintenance duplication if ID semantics change later.
Best fix: Optionally pass the already resolved input/output IDs along with actionIndex/config/fallback, or retain an explicit note about the shared deterministic resolver.
Severity: Low, non-blocking. Raised by another reviewer: Related to @SEZ9 F8; this adds the precise runtime-call-site distinction, rather than reasserting an unused getter.

Issue 4: Document the public ID resolver and dry-run scheduler boundary

Location: ConfigParserUtil.java:269-271; DryRunConnectValidator.java:228-235.
Problem: The now-public getInputIds method has no contract comment for omitted input versus an explicit empty list, and the new wrapper lacks a comment stating it translates scheduler failures while preserving the shared resolution semantics. Those distinctions control the compatibility fallbacks and are central to this change.
Risk: Future callers can conflate omission with an empty list or reintroduce dry-run/runtime divergence while changing the wrapper.
Best fix: Add concise method comments explaining the default-ID/explicit-empty contract and exception translation. No algorithm rewrite is needed.
Severity: Medium, required core-method documentation under the review rules. Raised by another reviewer: No; carryover omission acknowledged above.

2. Code-quality assessment

2.1 Code conventions

The scheduler class/method, Fenwick intent, cycle detection and fallback rationale have comments; new files have ASF headers. Issue 4 covers the remaining newly exposed/core wrapper contracts. The scoped whitespace check is clean. The scoped whitespace check produced no entries.

2.2 Test coverage and stability

Stability rating: Stable. ConfigParserUtilTest uses seeded Random(12079L), bounded legacy-fixture polling and exact evaluation/index assertions; no wall-clock performance threshold is used. Parser/CLI tests have preemptive timeout guards for the former hang. Factory creation-order state is synchronized and explicitly reset before order-sensitive assertions. JobExecutionIT uses try-with-resources and a 30-second guard for pre-submission self-cycle rejection. No local build, unit/integration test, container or service was run. Coverage includes explicit/implicit self-reference, duplicate default/explicit outputs, partial inputs, reverse declarations, terminal omitted/empty fallback, actual schemas/edges, index overflow, and seeded legacy-order comparison. The E2E addition exercises getLogicalDag through a real client setup; no job should be submitted for its invalid graph.

Current exact-head fork Build is still queued, with no conclusion. The prior Build failure is not the current run result. This head contains merged JDBC fixture/UID fix #12938 (ancestor check exit 0). The newer empty-provider baseline fix #12895 is not an ancestor, but there is no current failing lane demonstrating that issue on this head, so I am not requesting a speculative sync or unrelated patch. Let this run start/finish, or inspect its queue/approval state if it remains queued.

2.3 Documentation

EN/ZH incompatible-changes notes are present at EN:179 and ZH:153 and now include the savepoint resubmission/output implications. Previous Issue 1 is closed. The explanatory comment around partially available inputs closes previous Issue 2.

3. Architectural soundness

3.1 Solution quality

A long-term shared resolution path fixes both a hang and silent lineage loss, with narrow legacy compatibility instead of divergent ad hoc loops.

3.2 Maintainability

Centralized scheduling, explicit fallback state and behavioral regressions are strong boundaries; finish the two method comments and consider removing the redundant runtime ID read later.

3.3 Extensibility

Keep shared options linked to their actual definitions and connector-specific limits explicit. No new extension API is introduced.

3.4 Historical compatibility

No runtime migration, state rewrite or dependency replacement is introduced; examples must match the version being used.

4. Findings summary

Number	Finding	Location	Severity
4	Public resolver/wrapper contracts need comments	ConfigParserUtil.java:269; DryRunConnectValidator.java:228	Medium, required
3	Optional reuse of scheduled IDs in runtime parser	MultipleTableJobConfigParser.java:470-507	Low

Previous Issues 1-2 and SEZ9 F1-F8 technical questions are addressed.

5. Merge conclusion

Conclusion: Can merge after fixes

Blocking items: Issue 4's concise method documentation, then a completed current-head Build and effective write-capable maintainer approval. No algorithmic/source-behavior blocker was found.
Non-blocking recommendations: Issue 3 is optional cleanup. Keep source/CI conclusions separate and do not add unrelated connector fixes merely to chase a queued run.

I agree with the core direction and SEZ9's resolved technical points. A small documentation follow-up is sufficient on the code side; an alternative scheduler is unnecessary. The current Build must complete before approval can be treated as merge-ready.

Compare against dev: diverged, ahead 19 / behind 14, merge conflict=False. Drift is recorded independently; it does not itself justify a sync-first comment without an unrelated CI failure.

Daniel has READ permission. This COMMENT is a technical assessment, not an effective write-level approval. A write-capable maintainer must confirm repository gates and supply the effective approval.

Local evidence: exact-head fetch/checkout on seatunnel-review-12079, complete diff and source call-chain reads cited above. No runtime verification was claimed.

SEZ9 · 2026-10-26T03:57:35Z

Thanks for the follow-up on the F1–F8 points. Based on the latest automated pass at 9152a91fa278e7827e66b73c691e6f1e8f961abc, the direction looks right: a single indexed dependency scheduler shared by graph validation, runtime parsing and the connect dry-run (which is exactly what F5 asked for), waiting on every distinct dependency before plugin discovery, explicit cycle/unresolved rejection, and EN/ZH migration notes for the behavior change (F7).

A few things I still want to confirm before approving, since the review summary I have is truncated and I'd rather see it in the PR than assume:

F1 (duplicate output IDs): Please point me to the unit test that covers two or more transforms omitting plugin_output (both defaulting to the same ID) feeding a multi-input downstream, and confirm the scheduler now counts distinct dependency IDs so a dependent is released exactly once.
F2 / F4 (legacy fallback compatibility): Please link the test(s) showing a multi-transform job whose last transform omits plugin_input still resolves via the terminal fallback, and clarify which previously-passing multi-transform shapes (if any) now intentionally fail with ConfigCheckException. Whatever is intentionally rejected should be spelled out in the migration notes.
F3 / F8: Confirm that scheduling and validation in DryRunConnectValidator now derive input IDs from the same resolver (no separate getTransformInputIds vs getInputIds paths), and that the unused getOutputId() accessor and the redundant ReadonlyConfig re-parsing were removed or are now actually used.
F6 (single-transform self-cycle): Please confirm there is a test asserting that a single transform whose plugin_input equals its own output ID is rejected by the dry-run with the same error as the runtime parser, rather than passing through the legacy fallback.
F7 (docs): The automated pass also flags a remaining documentation gap for the newly public resolver/wrapper API. Please add that to the same docs change so the user-facing and developer-facing notes land together.

Once those are visible in the PR (tests + a short pointer to the doc sections), I'm happy to do the final pass.

DanielLeens · 2026-10-26T08:31:11Z

Hi @SEZ9 and @goutamadwant, thanks for the detailed questions. I re-read head 9152a91fa278 (static read only, nothing run locally). Here are the test pointers you asked for.

F1 (duplicate output IDs): ConfigParserUtilTest#testDuplicateOutputsReleaseEachMissingInputOnlyOnce (:356) and #testDuplicateOutputsCannotHideUnproducedInput (:260) cover the scheduler. MultipleTableJobConfigParserTest#testDuplicateUnnamedOutputsWaitForOtherJoinInput (:773) and SeaTunnelConfValidateCommandTest#testConnectDryRunDuplicateUnnamedOutputsWaitForOtherInput (:291) cover the runtime parser and the dry-run for two transforms omitting plugin_output that feed a multi-input downstream. Each output ID removes its waiting list once, so a dependent is released exactly once.
F2 / F4 (fallbacks): The terminal fallback is kept and tested in ConfigParserUtilTest#testTerminalOmittedInputRetainsLegacyFallback (:373), MultipleTableJobConfigParserTest#testTerminalOmittedInputUsesNamedTransformSchemaAndEdge (:728) and #testTransformEvaluationOrderKeepsLegacySinkFallbackOnTerminalTransform (:196). The shapes that now fail on purpose are covered by ConfigParserUtilTest#testDoesNotApplyLegacyFallbackAfterAnotherTransformWasScheduled (:175) and #testRejectsAmbiguousEmptyInputFallbacks (:194). Those shapes, and the savepoint-resubmission effect, are spelled out in the en and zh incompatible-changes entries.
F3 / F8: Input IDs come from the shared ConfigParserUtil.getInputIds in the scheduler, the runtime parser (MultipleTableJobConfigParser.java:492) and dry-run, so there is no second resolver. ScheduledTransform.getOutputId() is not unused: DryRunConnectValidator.validateTransform reads it. The runtime parser still re-reads the same IDs from config, which I rate as optional cleanup and not a correctness issue.
F6 (self-cycle): ConfigParserUtilTest#testSingleExplicitSelfCycleCannotUseLegacyFallback (:387), SeaTunnelConfValidateCommandTest#testConnectDryRunRejectsSingleExplicitSelfCycle (:260) and JobExecutionIT#testRejectsExplicitTransformSelfCycleBeforeSubmission cover it. Dry-run rethrows the scheduler failure as ConfigCheckException with the same message, so the wording matches the runtime JobDefineCheckException.
F7 (docs): The incompatible-changes entries are in place. You are right about the remaining gap, and it is the one item I still have open on this head: ConfigParserUtil.getInputIds (ConfigParserUtil.java:269, now public) and the new scheduleTransforms wrapper in DryRunConnectValidator.java:228 have no contract comments. A short Javadoc on each (omitted plugin_input defaults to the default ID versus an explicit empty list, and the wrapper only translates the exception type) would close it. No algorithm change is needed.

On CI: the Build for this head is a failure, but the one failing job I could inspect is Run / connector-file-local-it (17, ubuntu-latest). It stopped within about 1.5 minutes with Could not find or load main class org.apache.maven.wrapper.MavenWrapperMain, before any test ran, so it looks like a runner or Maven-wrapper bootstrap problem and not this diff. The other non-green job, connector-redis-it (11), was cancelled. Please rerun just those two failed jobs rather than the whole workflow. The branch also already contains the merged #12938 fix.

So the only item between this PR and a clean source review is the two method comments, plus a green rerun. Thanks again for the careful work!

SEZ9 · 2026-10-27T02:46:54Z

Thanks for the detailed pointers on 9152a91fa278. That mapping of each earlier point to specific tests and the incompatible-changes entries is helpful, and the description of the intended behaviour (each output ID releasing its waiting list once, the terminal fallback retained, a single shared input-ID resolver, self-cycle rejection with matching wording) lines up with what I was looking for.

I can't mark the findings resolved from the comment alone, though. I'll go through the diff on this head and confirm the tests and docs changes directly before updating the review.

Two things in the meantime:

Your comment appears to be cut off right at the open item you mentioned on ConfigParserUtil.getInputIds. Could you post the rest of that sentence, and say whether you plan to address it in this PR or in a follow-up?
On the duplicated ready-scheduler point: could you confirm explicitly where the dry-run validator and the runtime parser converge on a single scheduler implementation, or point me at the shared code? That drift concern was the core of that finding.

Once I've verified against the diff and have those two clarifications, I expect this to be close to ready.

DanielLeens · 2026-10-28T10:42:25Z

Thanks @SEZ9, and for taking the time to go through the diff on this head (9152a91fa278). Answers to your two questions (static read only, nothing run locally):

The "cut off" sentence. My previous comment is not truncated. The open item is simply the missing contract comments: ConfigParserUtil.getInputIds (now public) and the new DryRunConnectValidator.scheduleTransforms wrapper have no Javadoc on this head. The Javadoc should say that an omitted plugin_input defaults to the default ID, which is different from an explicit empty list, and that the wrapper only translates JobDefineCheckException into ConfigCheckException. It is comment-only, no algorithm change, and I would like it to land in this PR since the method visibility is changed here. @goutamadwant, could you add it when you next push?
Where runtime and dry-run converge. Both go through one implementation, TransformDependencyScheduler.scheduleTransforms(...) in seatunnel-engine-core (org.apache.seatunnel.engine.core.parse). The runtime path, MultipleTableJobConfigParser#parseTransforms, calls it directly (static import) with the transform configs and the current table keys, then iterates the returned ScheduledTransform list. The dry-run path, DryRunConnectValidator.scheduleTransforms, is a thin wrapper that calls the same method and only rethrows the failure as ConfigCheckException. Input IDs for both come from ConfigParserUtil.getInputIds via ScheduledTransform, so there is no second ready-queue or second resolver left in the diff. That is also why the unit tests in ConfigParserUtilTest exercise the scheduler once and the dry-run and runtime tests only check the wrapper and the parser behaviour.

I will re-verify once the Javadoc commit lands. The current Build run for this head was still in progress when I looked, so CI status is not yet confirmed either way.

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for adding the two method contracts. I re-read the full current diff at 67f3bde and the runtime/dry-run call chains. The previous required documentation finding is resolved, and I have no blocking production-source finding. Build remains in progress, with a completed connector-lane failure caused by a Docker Hub 502 during Spark image setup. One conditional test-isolation improvement and the earlier optional runtime-ID reuse remain non-blocking.

What Problem Does This PR Solve?

User pain point: Transform parsing could requeue unresolved dependencies forever, or construct a multi-input transform after only one upstream was available and silently omit other configured inputs.

Fix approach: Use a shared indexed scheduler for graph checks, runtime transform parsing and connect dry-run; wait for all distinct IDs, release each output ID once, diagnose cycles/unresolved dependencies and preserve the deliberately retained legacy bindings and action indexes.

One-sentence summary and example: A transform requiring sourceA and transformB now waits until both are available; an explicit self-cycle fails with a concrete cycle path rather than entering the legacy last-table fallback.

1. Code Change Review

1.1 Core Logic Analysis

The scheduler records missing distinct input IDs and ready/remaining config indexes. Circular TreeSet selection reproduces retry-queue evaluation order, while a Fenwick tree counts skipped remaining entries to preserve poll-derived action indexes without actually replaying them. Releasing waitingByInputId.remove(outputId) makes repeated producers satisfy an ID only once. After readiness is exhausted, Kahn cycle detection runs before the narrow terminal omitted/empty or single-transform total-mismatch fallback.

Before:

while (!remainingTransforms.isEmpty()) {
    // Filter unavailable inputs; retry when none are ready.
}

After:

List<ScheduledTransform> scheduled = scheduleTransforms(configs, availableOutputIds);
for (ScheduledTransform transform : scheduled) {
    // Every declared dependency is available, or the explicit legacy fallback applies.
}

Key Findings

Distinct missing IDs prevent repeated configured inputs from over-counting, and removal of the waiting list prevents duplicate outputs from releasing a join early.
Existing output values can be consumed before a same-ID overwrite, preserving in-place transform behavior.
Omitted plugin_input first resolves DEFAULT_ID; an explicit empty list stays empty. Only one remaining omitted/empty transform may fall back, or a single transform with no matched input after cycle rejection. Partially matched input lists do not qualify.
Cycle extraction is iterative and reports a concrete repeated path; it does not recurse with graph depth.
Long action indexes are range-checked before narrowing to int.
Runtime and dry-run deduplicate actual input actions/tables through linked sets while preserving their input order.
The newly public getInputIds and dry-run wrapper now document omission/empty semantics and exception translation.
Per-transform discovery/factory preparation follows readiness; the outer runtime parser still resolves connector jars/classloaders before its graph check, so this is not a claim that every bootstrap operation occurs after validation.

Complete runtime path

ClientJobExecutionEnvironment.getLogicalDag -> MultipleTableJobConfigParser.parse
 -> ConfigParserUtil.checkGraph -> shared scheduler / existing graph checks
 -> source discovery -> parseTransforms -> shared scheduler
 -> parseTransform -> input actions/tables -> transform factory -> downstream sink
connect dry-run -> DryRunConnectValidator.validateTransforms
 -> scheduleTransforms wrapper -> same scheduler -> scheduled IDs -> schema validation
savepoint resubmission -> configuration parse again -> same stricter validation

The dependency count tracks distinct unavailable IDs, while actual schemas are read in scheduled evaluation order, so overwrite behavior is not approximated by storing a schema in the scheduler. The circular remaining-index count reproduces unsuccessful legacy polls for valid graphs; it does not add a per-record cost. Cycles are checked before fallback, including a single explicit self-reference. Existing complex-graph explicit-ID and linkage requirements remain ahead of the shared cycle phase; direct parseTransforms/dry-run acceptance does not imply every omitted-ID graph passes the complete runtime graph validator.

1.2 Compatibility Impact

Partially incompatible. Previously cyclic/unresolved or partially bound configurations now fail instead of hanging or silently dropping upstreams. The two documented fallback families remain. Correctly resolved valid graphs retain evaluation order and default transform action names. Previously dropped upstreams can now affect output after a corrected or valid out-of-order graph is parsed, including savepoint resubmission. No job option, connector registration, serialized checkpoint format or wire protocol is removed.

1.3 Performance / Side-Effect Analysis

Scheduling is O((V+E) log V) with O(V+E) bookkeeping rather than repeated queue polling. The graph pre-check and runtime parse may each schedule once, a bounded parse-time cost. Kahn processing and iterative cycle extraction avoid graph-depth recursion. There are no added runtime threads, locks, network calls or per-record allocations. Very large poll-derived indexes fail explicitly rather than overflow.

1.4 Error Handling and Logging

Runtime throws JobDefineCheckException with unresolved output/input pairs or a concrete cycle. Dry-run translates to ConfigCheckException preserving message and cause, then retains existing contextual/sanitized reporting. Unknown upstream schemas remain SKIPPED rather than falsely validated. Missing scheduled inputs have a defensive runtime assertion before transform factory work.

Issue 1: Isolate the shared transform-creation recorder in tests

Location: seatunnel-core/seatunnel-starter/src/test/java/org/apache/seatunnel/core/starter/seatunnel/command/DryRunTestTransformFactory.java: CREATED_TABLES; SeaTunnelConfValidateCommandTest order assertions
Problem description: The test factory stores creation order in one static list. Individual add/reset/snapshot operations are synchronized, but a test's reset -> validation -> assertion sequence is not isolated from another test using the same factory.
Potential risk: If these methods are run concurrently, another test can clear or append to the recorder and make the exact-order assertions fail. No enabled parallel mode for this starter suite or observed CI failure from this mechanism was found; this is a conditional fixture risk, not a production defect.
Best improvement: Use a per-invocation recorder/correlation, or a shared test resource lock covering every use of this factory. Retain the actual order/schema assertions.
Severity: Medium; non-blocking.
Raised by another reviewer: No

Issue 2: Optional reuse of scheduled IDs in runtime parsing

Location: seatunnel-engine/seatunnel-engine-core/src/main/java/org/apache/seatunnel/engine/core/parse/MultipleTableJobConfigParser.java: parseTransforms/parseTransform
Problem description: Runtime passes the scheduled config/index/fallback but reads the input/output IDs again from immutable config; dry-run consumes the scheduled IDs directly. Both input reads use the same getInputIds helper, so this is currently redundant rather than inconsistent.
Potential risk: Small maintenance duplication if future ID resolution semantics change.
Best improvement: Optionally pass the scheduled input/output IDs into parseTransform in a focused cleanup; no scheduler rewrite is required.
Severity: Low; non-blocking.
Raised by another reviewer: Continuation of the earlier Daniel/SEZ9 F8 discussion; not an unused-getOutputId finding.

2. Code Quality Assessment

2.1 Coding Standards

The scheduler, Fenwick poll-count intent, cycle path extraction and fallback distinctions are explained in comments. The new public resolver and dry-run translation contracts now close the previous Medium documentation blocker. ASF headers and test-only factory service registration are present.

2.2 Test Coverage and Test Stability

Test stability: Risk present. Issue 1 records the conditional static-recorder isolation risk; the previous blanket Stable description was too broad for parallel reuse. Core tests use seeded Random(12079L), bounded legacy queue simulation, exact order/index assertions and an explicit integer-boundary case rather than a fragile speed threshold. Parser and dry-run tests assert actual schemas/edges, duplicate default outputs, reverse declarations, missing/partial inputs, omitted-vs-empty binding and cycle message text. Timeout guards bound former-hang checks. The added JobExecutionIT opens/closes a real client and checks getLogicalDag rejects the self-cycle before submission; it does not execute a successful transform job or prove checkpoint recovery. No local tests, containers or jobs were run.

Local analysis: fetched the head, created independent branch seatunnel-review-12079, read the complete 14-file diff and traced affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed, as requested. These are source-analysis conclusions; execution evidence is the current-head CI below.

2.3 Documentation Updates

English and Chinese migration entries describe strict failure, retained fallbacks, explicit-ID requirements, savepoint resubmission and changed downstream output when previously omitted inputs become connected. Required developer-facing method contracts are now present as well.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. Dependency readiness belongs in one engine-core scheduler shared by runtime parsing and dry-run, rather than separate retry-loop patches. The scheduler resolves order/binding, while callers retain schema/plugin responsibilities. The smallest safe fix and the preferred architectural boundary coincide. A wholesale DAG/state redesign would add unnecessary restore and compatibility risk.

3.2 Maintainability

The scheduler centralizes dependency and fallback policy. Fenwick bookkeeping is additional complexity justified by preserving legacy poll-based action names without quadratic retry work, with deterministic legacy-equivalence tests. Keep input-read cleanup optional and independent of correctness.

3.3 Extensibility

Future dependency sources or fallback policies must be added at this shared boundary and checked through both actual parser and dry-run schema tests. Keep semantic validation separate from merely naming an output ID.

3.4 Historical-Version Compatibility

The submitted running DAG and checkpoint representation are not rewritten. Resubmission from a savepoint reparses config and therefore applies the stricter behavior; the migration notes now state that. This source review does not claim an executed old-version restore matrix.

4. Issue Summary

Number	Issue	Location	Severity
1	Isolate the shared transform-creation recorder in tests	`seatunnel-core/seatunnel-starter/src/test/java/org/apache/seatunnel/core/starter/seatunnel/command/DryRunTestTransformFactory.java: CREATED_TABLES; SeaTunnelConfValidateCommandTest order assertions`	Medium
2	Optional reuse of scheduled IDs in runtime parsing	`seatunnel-engine/seatunnel-engine-core/src/main/java/org/apache/seatunnel/engine/core/parse/MultipleTableJobConfigParser.java: parseTransforms/parseTransform`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- No source blocker identified. No blocking production-source finding remains. Obtain green current-head CI and effective maintainer approval. The numbered follow-ups are non-blocking.
Recommended fixes — non-blocking
- Issue 1: Isolate the shared transform-creation recorder in tests; Issue 2: Optional reuse of scheduled IDs in runtime parsing

Overall assessment: The shared scheduler fixes the hang and partial-input loss while deliberately retaining supported legacy behavior. The previous required comment fix is complete. CI is still a gate even though the diagnosed connector failure occurs in image setup.

Existing-review reconciliation: +1 to SEZ9's F1-F8 checks. F1 is covered by one-time waiting-list removal plus ConfigParserUtilTest.testDuplicateOutputsReleaseEachMissingInputOnlyOnce, MultipleTableJobConfigParserTest.testDuplicateUnnamedOutputsWaitForOtherJoinInput and the matching dry-run test. F2/F4 are covered by terminal omitted/default binding tests and migration notes. F3/F5 converge on TransformDependencyScheduler and ConfigParserUtil.getInputIds. F6 dry-run explicitly asserts self -> self, matching runtime coverage. F7 now includes both migration and method contracts. F8 getOutputId has a production dry-run caller; runtime ID rereading is the optional Issue 2, not an unused API. Historical approval events are on earlier commits; no current effective independent approval is inferred. Author replies have been incorporated into this single full review.

Head 67f3bde2506aef2e6d0b3456f76e7ff59ad5932e. Live base dev at 3ac2d1cec6aa7b3741c4dacdd23020f778e23c02; comparison ahead, ahead 21 / behind 0; merge state BLOCKED. Behind/diverged alone is recorded metadata, not a request to synchronize upstream. Checks: Signal pull request update = SUCCESS; Build = IN_PROGRESS

Current fork run 41021951825 is still active. all-connectors-it-4, JDK 11 completed with one StarRocksIT.testStarRocksReadRowCount parameter-resolution error: Docker could not fetch a tyrantlucifer/spark image layer from registry-1.docker.io because it returned 502 Bad Gateway. This is before the test body/config parser; the relevant StarRocks/common fixture and workflow have no PR-owned diff. No verified upstream-sync cure is established. After checking the active run/attempt state, retry only that failed lane if still required; do not patch scheduler code or start an overlapping whole-workflow rerun. No rerun was triggered here. Aggregate Build remains IN_PROGRESS rather than green.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.

SEZ9 · 2026-10-31T04:02:29Z

Thanks for the follow-up on 67f3bde. Going through the earlier points against what is described so far:

PR12079-F1 (duplicate output IDs double-releasing joins): Counting distinct missing input IDs and removing the waitingByInputId entry per output ID so a repeated producer satisfies an ID only once is the fix I was after. Could you point me to the hunk and the test that covers two transforms sharing a default output ID feeding a multi-input transform, so I can confirm it on the diff?
PR12079-F6 (single-transform self-cycle slipping through the fallback): Running Kahn cycle detection before the terminal fallback sounds right. Is there a test asserting that an explicit self-cycle fails with a concrete cycle path instead of entering the fallback?
PR12079-F7 (user-facing behavior change without docs): Method contracts help, but the finding was about user-facing docs/changelog/upgrade notes for the new fail-fast rejection. Could you link the docs or release-note change, or let me know if you prefer to handle it separately?
PR12079-F5 (scheduler duplicated between dry-run and the Zeta parser): Please confirm the dry-run path no longer carries its own scheduler copy and delegates to the shared one.
PR12079-F2 / PR12079-F4 (legacy terminal fallback narrowed): Can you confirm a multi-transform job whose last transform omits plugin_input still dry-runs successfully via the terminal omitted/empty branch, ideally with a test covering that shape? That is the compatibility case I still can't tell from the summary.
PR12079-F3 / PR12079-F8 (scheduling vs. validation ID resolution, unused getOutputId()): Non-blocking from my side; if you leave them as is, a brief note on why dry-run keeps its own resolution helpers would be enough.

Concrete asks before I approve: (1) diff/test pointers for F1 and F6, (2) the multi-transform omitted-plugin_input confirmation or test for F2/F4, (3) a word on the docs/changelog plan for F7.

[Fix][Zeta] Resolve transform dependencies without hangs or dropped i…

8c733d7

…nputs

github-actions Bot added core SeaTunnel core module Zeta labels Sep 4, 2026

goutamadwant self-assigned this Sep 4, 2026

DanielLeens reviewed Sep 4, 2026

View reviewed changes

goutamadwant added 2 commits September 4, 2026 22:39

[Chore][Zeta] Merge upstream dev into transform dependency fix

e6c952d

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

[Docs][Zeta] Document transform dependency validation compatibility

6d82446

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

github-actions Bot added the document label Sep 5, 2026

DanielLeens reviewed Sep 5, 2026

View reviewed changes

DanielLeens reviewed Sep 11, 2026

View reviewed changes

Merge branch 'dev' into fix-zeta-transform-cycle

2af0068

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

DanielLeens reviewed Sep 15, 2026

View reviewed changes

DanielLeens suggested changes Sep 15, 2026

View reviewed changes

goutamadwant added 2 commits September 15, 2026 18:17

Merge branch 'dev' into fix-zeta-transform-cycle

3ad4966

[Fix][Zeta] Share transform scheduling and preserve legacy input binding

d796232

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

github-actions Bot added the e2e label Sep 16, 2026

DanielLeens approved these changes Sep 16, 2026

View reviewed changes

github-actions Bot added the reviewed label Sep 16, 2026

[Chore][Zeta] Merge dev 71860b4 into PR apache#12079

df0344d

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

[Chore][Core] Merge dev to include upstream CDC schema fix

6dcbe45

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

Merge remote-tracking branch 'upstream/dev' into fix-zeta-transform-c…

ad8cadd

…ycle Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

DanielLeens reviewed Oct 11, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/dev' into fix-zeta-transform-c…

612b5ed

…ycle

DanielLeens reviewed Oct 18, 2026

View reviewed changes

Merge branch 'dev' into fix-zeta-transform-cycle

b190936

DanielLeens reviewed Oct 20, 2026

View reviewed changes

nzw921rx mentioned this pull request Oct 22, 2026

[Roadmap] Apache SeaTunnel 3.1.0 Roadmap #12949

Open

Merge remote-tracking branch 'upstream/dev' into fix-zeta-transform-c…

3696a4b

…ycle

DanielLeens reviewed Oct 23, 2026

View reviewed changes

[Docs][Zeta] Explain restored transform-input migration

9152a91

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Oct 25, 2026

View reviewed changes

nzw921rx added the impact: high Engine, API, platform, CI, or a public core class / option contract of a connector label Oct 26, 2026

goutamadwant added 2 commits October 29, 2026 23:56

[Docs][Core] Clarify transform dependency scheduling contracts

3d642fe

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

[Chore][Zeta] Merge dev CI fixes into transform dependency validation

67f3bde

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Oct 31, 2026

View reviewed changes

Conversation

goutamadwant commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Current review evidence

Check list

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

Uh oh!

goutamadwant commented Sep 12, 2026

Uh oh!

DanielLeens commented Sep 13, 2026

Uh oh!

goutamadwant commented Sep 15, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

Before / After Example

Runtime Flow That Reaches The Changed Code

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

goutamadwant commented Sep 4, 2026 •

edited

Loading

1.1 Core Logic Analysis — verifying my own `CHANGES_REQUESTED` blocker against the new commit