DanielLeens commented on PR #12079: URL: https://github.com/apache/seatunnel/pull/12079#issuecomment-6383292056
Thanks @SEZ9 for the careful re-check on the synced head. On F8, I re-read head `3696a4b3c07d28fd0903174cac4968e561c02b67` (static read only, nothing run locally), and I also need to correct one detail in my earlier F8 note. Sorry for the imprecision there. **F8 on this head:** - **`getOutputId()` is still present and has a production caller, so it is not unused.** `ScheduledTransform.getOutputId()` (`TransformDependencyScheduler.java:247`) is read by `DryRunConnectValidator.validateTransform` (`DryRunConnectValidator.java:240`), next to `getInputIds()` (`:242`). Tests also use it (`ConfigParserUtilTest`, `SeaTunnelConfValidateCommandTest`). - **My correction:** I said earlier that `MultipleTableJobConfigParser` also calls `getOutputId()`. It does not. `parseTransforms` (`MultipleTableJobConfigParser.java:470-480`) passes only `getActionIndex()`, `getConfig()` and `isLegacyFallback()` into `parseTransform`, which then derives the IDs from `ReadonlyConfig` again: `getInputIds(readonlyConfig)` at `:492` and `PLUGIN_OUTPUT ... orElse(DEFAULT_ID)` at `:507` / `:522`. - **Consolidation:** input IDs are resolved through the shared `ConfigParserUtil.getInputIds` helper in all three places: the scheduler constructor (`TransformDependencyScheduler.java:235`, now `public` for this reason), the runtime parser (`:492`) and dry-run. The old static output-id helper is gone. The output ID uses the same `getOptional(PLUGIN_OUTPUT).orElse(DEFAULT_ID)` expression everywhere, because `ConfigParserUtil` has no output-id helper. All paths compute IDs the same deterministic way, so the scheduler and the runtime parser cannot disagree. The second read in the runtime path is redundant, but it is not a correctness issue. Optional cleanup for @goutamadwant (not blocking): pass `scheduledTransform.getInputIds()` / `getOutputId()` into `parseTransform` instead of re-reading the config. The runtime path then reads IDs exactly once, as dry-run already does. A short note that the current form is intentional is equally fine. For completeness: the `Build` check on `3696a4b3c07d` is currently reported as `FAILURE`, so the current-head CI gate is still open. My source-level conclusion from the latest review is unchanged. -- 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]
