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