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