sharpca commented on PR #1337: URL: https://github.com/apache/flink-agents/pull/1337#issuecomment-6578087677
Wenjin asked me to coordinate with you on scope, since #1347 is the Python counterpart of this change (it adds flat `agent.*` support on the Python side). I read the current head `89ae183`. Two of the three rules clarified in the discussion aren't visible in that diff yet, so I'd like to settle them with you before I mirror anything into Python: 1. **Selection order and version branch.** The diff currently selects `config.yaml` and falls back to `flink-conf.yaml` only when `config.yaml` is missing, with no branch on the Flink version. The clarified rules are the other way round for 1.20 (prefer `flink-conf.yaml`, fall back to `config.yaml`) and stricter for 2.x (`config.yaml` only). 2. **Legacy parser.** Both files still go through `YamlParserUtils.loadYamlFile`, so `flink-conf.yaml` has no legacy parsing path yet — that is the part that matters for values containing `:`. There is also an existing asymmetry worth noting: the Python loader (`__find_config_file` in `python/flink_agents/runtime/remote_execution_environment.py`) tries `flink-conf.yaml` **first** and then `config.yaml`, so Java and Python disagree on selection today, before the new rules are applied at all. Three questions I need answered to make the Python side match rather than guess: - **Where does the version decision come from on the Java side?** Python has `flink_agents.api.version_compatibility.FlinkVersionManager`, so I can branch on the installed Flink version, but I want to use the same input you do. - **Is the legacy parser planned inside #1337 or as a follow-up?** And which edge cases must match `GlobalConfiguration.loadLegacyYAMLResource()` — values containing `:`, unquoted scalars, comments, duplicate keys, nested indentation being rejected? - **One PR or two?** If you keep Java here, I'll implement the selection rules plus a small legacy parser in #1347 with tests mirroring your cases. If you'd rather keep Java and Python together, I'll contribute the Python part here and close #1347 instead. I'm deliberately not pushing a parser to #1347 before we agree on the above — a Python parser that diverges from your Java semantics would be worse than the current state. cc @wenjin272 for visibility. -- 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]
