DanielLeens commented on PR #12079: URL: https://github.com/apache/seatunnel/pull/12079#issuecomment-5846270888
Thanks for holding the line on verifying against the diff rather than the summary, @SEZ9 — that's exactly the right instinct, and here's the current-head evidence for all seven: 1. **F8** — `DryRunConnectValidator.java:214-220` (`validateTransforms`) now passes the whole `scheduledTransform` into `validateTransform`, and `validateTransform` (`:233-244`) reads `outputId = scheduledTransform.getOutputId()` and `inputIds = scheduledTransform.getInputIds()` instead of re-parsing `ReadonlyConfig`. The new getter is `TransformDependencyScheduler.java:251-253`. I grepped the full tree at this head for the old `getTransformInputIds`/`getTransformOutputId` statics — zero hits, so nothing redundant is left. 2. **F7** — both `docs/en/introduction/concepts/incompatible-changes.md` (under "Transform Dependency Resolution") and its `docs/zh` counterpart (the equivalent section, translated) carry the entry, both explicitly referencing `(#12079)` and describing the fail-fast rejection of unresolved/cyclic transform graphs plus the migration guidance. Neither file changed in the `d8045271fcf6 -> 3df75448c` commit, but they were already present from the earlier round and I re-checked them at the current head rather than assuming. 3. **F5** — one implementation, no remaining copy. `ConfigParserUtil.checkGraph`'s two branches (`ConfigParserUtil.java:73` and `:180`) and `MultipleTableJobConfigParser.parseTransforms` (`MultipleTableJobConfigParser.java:471`) all call `TransformDependencyScheduler.scheduleTransforms` directly. `DryRunConnectValidator.scheduleTransforms` (`DryRunConnectValidator.java:224-227`) is a 6-line wrapper around that same static call that only translates `JobDefineCheckException` into `ConfigCheckException` for the CLI's error type — it carries no scheduling logic of its own. 4. **F1** — `TransformDependencyScheduler.java:116`: `waitingByInputId.remove(transform.outputId)`. Using `remove` (not `get`/`getOrDefault`) empties the map entry the first time any producer of that output ID is scheduled, so a second transform producing the same `plugin_output` finds nothing left to release — each dependent's `unresolvedInputCount` is decremented exactly once per distinct ID, never once per producer. This line is unchanged since `d8045271fcf6`; the `d8045271fcf6 -> 3df75448c` commit only touched `DryRunConnectValidator`'s ID lookup (F8) and added the `getInputIds()` getter, nothing in the scheduling algorithm itself. Tests at the current head: `ConfigParserUtilTest.java:356` (`testDuplicateOutputsReleaseEachMissingInputOnlyOnce`), `MultipleTableJobConfigParserTest.java:773` (`testDuplicateUnnamedOutputsWaitForOtherJoinInput` — this class actually lives in `seatunnel-engine-client`, correcting an earlier round where I said `seatunnel-engine-core`), and the dry-run mirror `SeaTunnelConfValidateCommandTest.java:291` (`testConnectDryRunDuplicateUnnamedOutputsWaitForOtherInput`). 5. **F2/F4** — `TransformDependencyScheduler.java:140`: `emptyInputFallback = transform.inputIds.isEmpty() || transform.inputOmitted`, where `inputOmitted` (`:236`) is `!readonlyConfig.getOptional(PLUGIN_INPUT).isPresent()`. That `||` is what restores the legacy last-inserted-table fallback for an *omitted* `plugin_input` regardless of how many transforms are in the job — it's no longer limited to single-transform jobs, and it's distinct from an explicit `plugin_input: []`. Tests: `MultipleTableJobConfigParserTest.java:728` (`testTerminalOmittedInputUsesNamedTransformSchemaAndEdge`) and `:733` (`testOmittedInputPrefersExistingDefaultOverLastNamedOutput`), with dry-run mirrors `SeaTunnelConfValidateCommandTest.java:245` and `:273`. 6. **F3** — one shared path. `ConfigParserUtil.getInputIds` (`ConfigParserUtil.java:269`) is the only place input IDs are resolved from `ReadonlyConfig`; `ScheduledTransform`'s constructor calls it directly (`TransformDependencyScheduler.java:235`), and after the F8 change `DryRunConnectValidator` reads the already-resolved `scheduledTransform.getInputIds()` rather than calling any parser itself. Validation and scheduling can't diverge because they're the same computation now, not two call sites of the same helper. 7. **F6** — `TransformDependencyScheduler.java:292`: the cycle-detection edge builder only skips an input-equals-output pair when `transform.inputOmitted` is true (the implicit legacy chain case). An *explicit* self-reference (`inputOmitted=false`) is kept as a real graph edge with in-degree 1, which Kahn's algorithm can never drain, so it falls through to the `"Transform dependency cycle detected: x -> x"` exception at `:337-340` — in dry-run too, since `checkGraph`'s simple-graph branch now always calls `scheduleTransforms`/cycle detection before falling back. Tests: `ConfigParserUtilTest.java:221` (`testSimpleGraphRejectsExplicitSelfCycle`), `:387` (`testSingleExplicitSelfCycleCannotUseLegacyFallback`), and `:240` (`testImplicitSingleTransformIsNotAnExplicitSelfCycle`, the negative case showing implicit chaining is *not* flagged), plus the dry-run mirror `SeaTunnelConfValidateCommandTest.java:260` (`testConnectDryRunRejectsSingleExplicitSelfCycle`) and the client-to-master e2e `JobExecutionIT.java:90` (`testRejectsExplicitTransformSelfCycleBeforeSubmission`, time-bounded via `assertTimeoutPreemptively` at `:101` so a regression to the old hang would fail the test rather than hang CI). All seven check out against the actual `3df75448c` source, not just against the summary. Separately (unprompted, but worth flagging since it landed after my last review): the `Build` check on this head has since completed and shows `FAILURE`. I traced it into the fork's run — one failing job (`engine-v2-it`) is the pre-existing `BackpressureSlowSinkIT` checkpoint-timing flake that's unrelated to this change, and the other (`transform-v2-it-part-1`) fails inside a "Run SeaTunnel on spark" step, which is on the Spark execution path this PR doesn't touch (the Zeta-only scheduler/validator changes here don't run through Spark's translation layer). I'd still recommend a fresh CI run to confirm before merge, but from the diff itself I don't see anything left open. === END REPLY === -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
