wenjin272 commented on code in PR #1337:
URL: https://github.com/apache/flink-agents/pull/1337#discussion_r4409410252
##########
runtime/src/main/java/org/apache/flink/agents/runtime/env/RemoteExecutionEnvironment.java:
##########
@@ -119,10 +127,34 @@ public static AgentConfiguration
loadAgentConfiguration(String configDir) {
if (configDir == null) {
return new AgentConfiguration();
}
- final Map<String, Object> configData =
- (Map<String, Object>)
- YamlParserUtils.loadYamlFile(new File(configDir,
FLINK_CONF_FILENAME))
- .getOrDefault("agent", new HashMap<>());
+ File configFile = new File(configDir, FLINK_CONF_FILENAME);
Review Comment:
Could we align configuration loading across Java and Python based on the
Flink version? For Flink 1.20, prefer `flink-conf.yaml` and fall back to
`config.yaml`; for Flink 2.0+, read only `config.yaml`. We can reuse Python’s
`flink_version_manager` and Flink’s `EnvironmentInformation.getVersion()` in
Java. Please also cover the file-selection combinations in tests.
##########
runtime/src/main/java/org/apache/flink/agents/runtime/env/RemoteExecutionEnvironment.java:
##########
@@ -119,10 +127,34 @@ public static AgentConfiguration
loadAgentConfiguration(String configDir) {
if (configDir == null) {
return new AgentConfiguration();
}
- final Map<String, Object> configData =
- (Map<String, Object>)
- YamlParserUtils.loadYamlFile(new File(configDir,
FLINK_CONF_FILENAME))
- .getOrDefault("agent", new HashMap<>());
+ File configFile = new File(configDir, FLINK_CONF_FILENAME);
+ if (!configFile.exists()) {
+ File legacyConfigFile = new File(configDir,
LEGACY_FLINK_CONF_FILENAME);
+ if (legacyConfigFile.exists()) {
+ LOG.warn(
+ "{} not found in {}, using legacy config file {}",
+ FLINK_CONF_FILENAME,
+ configDir,
+ LEGACY_FLINK_CONF_FILENAME);
+ configFile = legacyConfigFile;
+ }
+ }
+
+ final Map<String, Object> yaml =
YamlParserUtils.loadYamlFile(configFile);
Review Comment:
Could we use legacy parsing rules for `flink-conf.yaml`? The standard YAML
parser rejects valid legacy configurations, including duplicate keys and
literal values containing `: `. I reproduced both cases: Flink 1.20.3 loads
them successfully, while this PR throws `YamlEngineException`, even when the
offending entries are unrelated to Agents. Please add regression tests for
these cases.
--
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]
