You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/01/31 00:15:16 UTC
[GitHub] [samza] kw2542 opened a new pull request #1265: SAMZA-2441: Update
ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file
kw2542 opened a new pull request #1265: SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file
URL: https://github.com/apache/samza/pull/1265
Design:
https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner
Changes:
1. Override ApplicationRunnerCommandLine#loadConfig not to invoke config loader as ApplicationRunners handles initial config and full job config themselves.
2. Add JobRunnerCommandLine to load full job config for JobRunner as it still needs full job config for special use cases.
API Changes:
None
Upgrade Instructions:
None
Usage Instructions:
None
Tests:
1. Unit Tests
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] cameronlee314 commented on a change in pull request #1265:
SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not
to load local file
Posted by GitBox <gi...@apache.org>.
cameronlee314 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_r374938467
##########
File path: samza-core/src/test/java/org/apache/samza/runtime/TestApplicationRunnerMain.java
##########
@@ -79,6 +82,28 @@ public void TestStatusOperation() throws Exception {
assertEquals(1, TestApplicationRunnerInvocationCounts.statusCount);
}
+ @Test
+ public void TestLoadConfig() {
+ ApplicationRunnerMain.ApplicationRunnerCommandLine cmdLine = new ApplicationRunnerMain.ApplicationRunnerCommandLine();
+ OptionSet options = cmdLine.parser().parse(
+ "--config-loader-factory",
+ "org.apache.samza.config.loaders.PropertiesConfigLoaderFactory",
+ "--config-loader-properties",
+ "path=" + getClass().getResource("/test.properties").getPath(),
+ "-config", String.format("%s=%s", ApplicationConfig.APP_CLASS, MockStreamApplication.class.getName()),
+ "-config", String.format("app.runner.class=%s", TestApplicationRunnerInvocationCounts.class.getName()));
+
+ Config actual = cmdLine.loadConfig(options);
+
+ assertEquals(4, actual.size());
+ assertEquals("org.apache.samza.config.loaders.PropertiesConfigLoaderFactory", actual.get(JobConfig.CONFIG_LOADER_FACTORY));
+ assertEquals(
+ getClass().getResource("/test.properties").getPath(),
+ actual.get(ConfigLoaderFactory.CONFIG_LOADER_PROPERTIES_PREFIX + "path"));
+ assertEquals(MockStreamApplication.class.getName(), actual.get(ApplicationConfig.APP_CLASS));
+ assertEquals(TestApplicationRunnerInvocationCounts.class.getName(), actual.get("app.runner.class"));
Review comment:
Minor suggestion: Instead of needing to check the size and each individual entry, could you build the expected config map and then do an equals check?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] kw2542 commented on a change in pull request #1265:
SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not
to load local file
Posted by GitBox <gi...@apache.org>.
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_r374951906
##########
File path: samza-core/src/test/java/org/apache/samza/runtime/TestApplicationRunnerMain.java
##########
@@ -79,6 +82,28 @@ public void TestStatusOperation() throws Exception {
assertEquals(1, TestApplicationRunnerInvocationCounts.statusCount);
}
+ @Test
+ public void TestLoadConfig() {
+ ApplicationRunnerMain.ApplicationRunnerCommandLine cmdLine = new ApplicationRunnerMain.ApplicationRunnerCommandLine();
+ OptionSet options = cmdLine.parser().parse(
+ "--config-loader-factory",
+ "org.apache.samza.config.loaders.PropertiesConfigLoaderFactory",
+ "--config-loader-properties",
+ "path=" + getClass().getResource("/test.properties").getPath(),
+ "-config", String.format("%s=%s", ApplicationConfig.APP_CLASS, MockStreamApplication.class.getName()),
+ "-config", String.format("app.runner.class=%s", TestApplicationRunnerInvocationCounts.class.getName()));
+
+ Config actual = cmdLine.loadConfig(options);
+
+ assertEquals(4, actual.size());
+ assertEquals("org.apache.samza.config.loaders.PropertiesConfigLoaderFactory", actual.get(JobConfig.CONFIG_LOADER_FACTORY));
+ assertEquals(
+ getClass().getResource("/test.properties").getPath(),
+ actual.get(ConfigLoaderFactory.CONFIG_LOADER_PROPERTIES_PREFIX + "path"));
+ assertEquals(MockStreamApplication.class.getName(), actual.get(ApplicationConfig.APP_CLASS));
+ assertEquals(TestApplicationRunnerInvocationCounts.class.getName(), actual.get("app.runner.class"));
Review comment:
Good point, updated.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] kw2542 commented on a change in pull request #1265:
SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not
to load local file
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [samza] lhaiesp merged pull request #1265: SAMZA-2441: Update
ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file
Posted by GitBox <gi...@apache.org>.
lhaiesp merged pull request #1265: SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file
URL: https://github.com/apache/samza/pull/1265
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] cameronlee314 commented on a change in pull request #1265:
SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not
to load local file
Posted by GitBox <gi...@apache.org>.
cameronlee314 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_r374937759
##########
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:
Do you feel that the "base" use case for `CommandLine` is to need the full config? To me, it looks a little awkward to need to override `loadConfig` here with an implementation which is almost the same as the base `loadConfig`.
I suppose one alternative would be to make this impl here the base impl, and then have each use case call `ConfigUtil.loadConfig` if is needs it. What do you see as the tradeoffs?
----------------------------------------------------------------
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
With regards,
Apache Git Services