[GitHub] [flink] XComp commented on a change in pull request #13747: [FLINK-19662][runtime] Exposing the JobManager's memory configuration through /jobmanager/config

[GitBox <gi...@apache.org>]

XComp commented on a change in pull request #13747:
URL: https://github.com/apache/flink/pull/13747#discussion_r516107826



##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/entrypoint/YarnEntrypointUtils.java
##########
@@ -51,8 +56,24 @@
  */
 public class YarnEntrypointUtils {
 
-	public static Configuration loadConfiguration(String workingDirectory, Map<String, String> env) {
-		Configuration configuration = GlobalConfiguration.loadConfiguration(workingDirectory);
+	private static final Logger LOG = LoggerFactory.getLogger(YarnEntrypointUtils.class);
+
+	public static Configuration loadConfiguration(String workingDirectory, String[] args, Map<String, String> env) {
+		Configuration dynamicParameters = null;
+		final CommandLineParser<Configuration> commandLineParser = new CommandLineParser<>(
+				new DynamicParametersConfigurationParserFactory());
+
+		try {
+			dynamicParameters = commandLineParser.parse(args);
+		} catch (FlinkParseException e) {
+			LOG.error("Could not parse command line arguments {}.", args, e);
+			commandLineParser.printHelp(StandaloneSessionClusterEntrypoint.class.getSimpleName());
+			System.exit(1);
+		}

Review comment:
       You have a good point. I looked into the code again and realized that it's not ideal (the current version and your proposal moving everything in a single utility method). I don't consider calling `System.exit(..)` within a utility method good coding. Such a call should be located in the main method. 
   
   Moving the left-over code into a utility method would have meant splitting up the error handling (since `CommandLineParser.printHelp(..)` would have still been called within the utility method).
   
   I didn't consider splitting up the error handling being a good approach either. Hence, I moved the dynamic parameter parsing into each main method in [7f722c9](https://github.com/apache/flink/pull/13747/commits/7f722c99b83591793147b2dda952a3acadf5f2b7). What's your take on this?
   
   FYI: The commit above also fixed a bug that used the wrong class in `CommandLineParser.printHelp(..)`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org