kw2542 commented on a change in pull request #1265: SAMZA-2441: Update
ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file
URL: https://github.com/apache/samza/pull/1265#discussion_r374964499
##########
File path:
samza-core/src/main/java/org/apache/samza/runtime/ApplicationRunnerMain.java
##########
@@ -43,9 +49,33 @@ public ApplicationRunnerOperation getOperation(OptionSet
options) {
String rawOp = options.valueOf(operationOpt);
return ApplicationRunnerOperation.fromString(rawOp);
}
+
+ @Override
+ public Config loadConfig(OptionSet options) {
Review comment:
Good question, in all 13 direct usage of CommandLine, and 5 sub classes of
CommandLine, they are all expecting full job configs to work properly, e.g.
RocksDbReadingTool extends CommandLine to read full job config before being
able to read data from Rocks DB, which is being invoked by CLI.
If we change CommandLine to the other way, that means we need to invoke
ConfigUtil.loadConfig explicitly in 13 places, and override loadConfigs in 5
sub classes to invoke ConfigUtil.loadConfig too, compared to overriding once in
ApplicationRunnerCommandLine only.
I would prefer the current way to keep the base class to serve majority use
case.
----------------------------------------------------------------
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]
With regards,
Apache Git Services
