shanthoosh commented on a change in pull request #920: SAMZA-2106: Samza app
and job config refactor
URL: https://github.com/apache/samza/pull/920#discussion_r258141427
##########
File path: samza-core/src/main/java/org/apache/samza/execution/JobPlanner.java
##########
@@ -155,4 +158,33 @@ final void writePlanJsonFile(String planJson) {
systemStreamConfigs.put(JobConfig.JOB_DEFAULT_SYSTEM(),
dsd.getSystemName()));
return systemStreamConfigs;
}
+
+ /**
+ * Generates job.id from app.id and job.name from app.name config
+ * If both job.id and app.id is defined, app.id takes precedence and job.id
is set to value of app.id
+ * If both job.name and app.name is defined, app.name takes precedence and
job.name is set to value of app.name
+ *
+ * @param allowedUserConfig configs passed from user
+ */
+ void generateJobIdAndName(Map<String, String> allowedUserConfig) {
+ if (allowedUserConfig.containsKey(JobConfig.JOB_ID())) {
+ LOG.warn("{} is a deprecated configuration, use app.id instead.",
JobConfig.JOB_ID());
Review comment:
1. Can you please deprecate the job.id, job.name configurations from the
samza configuration table.
2. There're some samples in
[samza-hello-samza](https://github.com/apache/samza-hello-samza/blob/master/src/main/config/azure-application-local-runner.properties)
in which job.id, job.name are defined as configuration. Could you please
update them to use app.id, app.name. Could you check if the same point applies
to the samza-hello-samza samples within linkedin?
3. Could you please update the integration test files located in
samza-test/src/test/ to use the new app.id, app.name configuration.
[Example](https://github.com/apache/samza/blob/master/samza-test/src/main/config/join/emitter.samza)
4. From what I see, we're doing configuration aliasing in the
`LocalApplicationRunner`, `JobPlanner` directly. Could you share what is the
rationale behind this decision? Why not do this in a `Rewriter`?
5. Consider a case where user has defined only `app.id`, `app.name` in the
configuration and launches their low-level application. Could you please share
how will submitting `YarnJob` through `JobRunner` work? `run-job.sh` launches
`YarnJob` which requires the `job.name`, `job.id` to built through
`CoordinatorStreamUtil.buildCoordinatorStreamConfig` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
