SEZ9 commented on PR #12079: URL: https://github.com/apache/seatunnel/pull/12079#issuecomment-6594169443
Thanks for the update on head `67f3bde2506aef2e6d0b3456f76e7ff59ad5932e`. The points from my earlier review still stand, so I'd like to see them addressed (or explicitly discussed) before this moves forward: **Blocking (HIGH)** - **PR12079-F1 (duplicate transform output IDs):** multiple transforms that omit `plugin_output` all default to `DEFAULT_ID`, so the ready-scheduler double-decrements `unresolvedInputCount` and releases multi-input dependents prematurely, turning a resolvable DAG into a "missing scheduled inputs" hard failure. Please de-duplicate/validate output IDs before scheduling and add a test covering two transforms with default output IDs feeding a shared downstream. - **PR12079-F2 (legacy `findLast` fallback narrowed):** multi-transform jobs whose last transform omits `plugin_input` used to resolve via the terminal fallback and now break. Please restore the fallback for that case or justify the behavior change explicitly. **Should fix (MEDIUM)** - **PR12079-F3:** scheduling uses `getTransformInputIds` while validation uses `getInputIds`; please converge on one helper so a divergence can't produce a confusing failure. - **PR12079-F4:** related to F2 — multi-transform configs that previously passed via the implicit last-schema fallback now hard-fail with `ConfigCheckException`; this needs a migration path or explicit justification. - **PR12079-F6:** a single-transform self-cycle still passes dry-run via the legacy fallback, which contradicts the cycle-detection goal and diverges from the runtime parser. Please make dry-run reject it as well. - **PR12079-F5:** the indexed ready-scheduler is duplicated between `DryRunConnectValidator` and `MultipleTableJobConfigParser`; extracting a shared implementation would avoid drift. Happy for this to be a follow-up if you'd rather keep this PR focused — just say so. - **PR12079-F7:** the fail-fast rejection of cyclic/unresolved transform graphs is a user-visible change and should get a docs/upgrade-note mention. **Nit (LOW)** - **PR12079-F8:** the new static helpers re-parse `ReadonlyConfig` and duplicate existing input/output-ID resolution; `getOutputId()` appears unused in production code and can be dropped or wired in. Could you push a revision covering F1 and F2 with tests, and either address or reply inline on the MEDIUM items? I'll take another look once that's up. <!-- streview-comment:2243 --> -- 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]
