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:
[email protected]
