SAMZA-2408: Update RemoteApplicationRunner to submit job only #1247
Conversation
Design: https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner Changes: 1. Depending on the existence of job.config.loader.factory, RemoteApplicationRunner will alternatively only submit the job, without planning. 2. Late initialize RemoteJobPlanner only when needed to avoid executing user code in submitting only mode. API Changes: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Upgrade Instructions: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Usage Instructions: N/A. This is part of a series PRs, detailed information will be provided in the last/main PR. Tests: N/A
samza-core/src/main/java/org/apache/samza/runtime/RemoteApplicationRunner.java
Outdated
Show resolved
Hide resolved
|
Change LGTM. Can you include a unit test for the behavior? |
| } | ||
|
|
||
| @Test | ||
| public void testRunWithConfigLoaderFactoryPresent() { |
There was a problem hiding this comment.
Hmm. I guess the code as-is isn't very unit test friendly. It would be nice to test the invocation of .submit vs .run on JobRunner depending on the presence of the ConfigLoaderFactory.
Maybe we could factor out the JobRunner creation into its own method, and use a spy to stub out the return value of that method to be a mock JobRunner that we can verify? Disclaimer though, I'm not sure what the best practices are of using spies...
There was a problem hiding this comment.
Haha, I figured that too. In general, I prefer not to mess with production code for the sake of unit tests.
The current unit actually test the new flow, I passed null SamzaApplication to RemoteApplicationRunner's constructor. In RemoteApplicationRunner#run, if it goes the legacy flow, it will throw exception when constructing RemoteJobPlanner where SamzaApplication is required. Do you think this is enough?
There was a problem hiding this comment.
There is some precedence elsewhere in samza code; ZkJobCoordinator.readJobModelFromMetadataStore for example uses a similar method structure + spy in unit test.
You are correct that the test written now does ensure that the legacy flow isn't triggered, but it does little to verify the behavior of the new flow. I won't block the PR on this though.
There was a problem hiding this comment.
+1 that there is a bit of missing test coverage here. It would be nice if this class was set up better for improved testing, but I also won't block the PR on this.
bkonold
left a comment
bkonold
left a comment
There was a problem hiding this comment.
LGTM, but make sure you take a look at the CI results.
samza-core/src/main/java/org/apache/samza/runtime/RemoteApplicationRunner.java
Show resolved
Hide resolved
…atus and waitForFinish. This is also used in ExecutionPlanner as well before.
…#1247) 1. Depending on the existence of job.config.loader.factory, RemoteApplicationRunner will alternatively only submit the job, without planning. 2. Late initialize RemoteJobPlanner only when needed to avoid executing user code in submitting only mode.
4 participants
Design:
https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner
Changes:
API Changes:
N/A. This is part of a series PRs, detailed information will be provided in the last/main PR.
Upgrade Instructions:
N/A. This is part of a series PRs, detailed information will be provided in the last/main PR.
Usage Instructions:
N/A. This is part of a series PRs, detailed information will be provided in the last/main PR.
Tests:
End to end tested with Hello Samza job.