SEZ9 commented on PR #12079: URL: https://github.com/apache/seatunnel/pull/12079#issuecomment-5674518763
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. <!-- streview-comment:1069 --> -- 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]
