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/08/27 21:27:24 UTC

[GitHub] [samza] kw2542 opened a new pull request #1424: SAMZA-2584: Refactor ClusterBasedJobCoordinator

kw2542 opened a new pull request #1424:
URL: https://github.com/apache/samza/pull/1424


   Issues: In the deployment flow of a beam job, we will have a complicate flow: ClusterBasedJobCoordinator#main -> Beam main class -> JobCoordinatorLaunchUtil -> ClusterBasedJobCoordinator.
   Changes:
     1. Move ClusterBasedJobCoordinator#main to ClusterBasedJobCoordinatorRunner#main
     2. Update run-jc.sh to invoke ClusterBasedJobCoordinatorRunner
   Tests:
     1. unit tests
     2. Deployed hello samza job successfully with the change following instructions on http://samza.apache.org/startup/hello-samza/latest/
   API Changes: None
   Upgrade Instructions: None
   Usage Instructions: None


----------------------------------------------------------------
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

[GitHub] [samza] mynameborat commented on pull request #1424: SAMZA-2584: Refactor ClusterBasedJobCoordinator

Posted by GitBox <gi...@apache.org>.
mynameborat commented on pull request #1424:
URL: https://github.com/apache/samza/pull/1424#issuecomment-685230550


   I don't see any changes to the `run-jc.sh`. Did you forget to include that change as part of the PR?


----------------------------------------------------------------
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

[GitHub] [samza] kw2542 commented on pull request #1424: SAMZA-2584: Refactor ClusterBasedJobCoordinator

Posted by GitBox <gi...@apache.org>.
kw2542 commented on pull request #1424:
URL: https://github.com/apache/samza/pull/1424#issuecomment-685850254


   > I don't see any changes to the `run-jc.sh`. Did you forget to include that change as part of the PR?
   
   Nice catch, just 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

[GitHub] [samza] mynameborat commented on pull request #1424: SAMZA-2584: Refactor ClusterBasedJobCoordinator

Posted by GitBox <gi...@apache.org>.
mynameborat commented on pull request #1424:
URL: https://github.com/apache/samza/pull/1424#issuecomment-685957501


   Can you fix the rat check for the new file?
   ```> Task :rat
   Rat report: build/rat/rat-report.html
   Unknown license: /home/travis/build/apache/samza/samza-core/src/test/java/org/apache/samza/clustermanager/TestClusterBasedJobCoordinatorRunner.java
   > Task :rat FAILED
   FAILURE: Build failed with an exception.
   * Where:
   Script '/home/travis/build/apache/samza/gradle/rat.gradle' line: 81
   ```


----------------------------------------------------------------
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

[GitHub] [samza] mynameborat merged pull request #1424: SAMZA-2584: Refactor ClusterBasedJobCoordinator

Posted by GitBox <gi...@apache.org>.
mynameborat merged pull request #1424:
URL: https://github.com/apache/samza/pull/1424


   


----------------------------------------------------------------
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

[GitHub] [samza] mynameborat commented on a change in pull request #1424: SAMZA-2584: Refactor ClusterBasedJobCoordinator

Posted by GitBox <gi...@apache.org>.
mynameborat commented on a change in pull request #1424:
URL: https://github.com/apache/samza/pull/1424#discussion_r482221639



##########
File path: samza-core/src/test/java/org/apache/samza/clustermanager/TestClusterBasedJobCoordinatorRunner.java
##########
@@ -64,15 +64,15 @@
 
 
 /**
- * Tests for {@link ClusterBasedJobCoordinator}
+ * Tests for {@link TestClusterBasedJobCoordinatorRunner}
  */
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({
     CoordinatorStreamUtil.class,
-    ClusterBasedJobCoordinator.class,
+    ClusterBasedJobCoordinatorRunner.class,
     CoordinatorStreamStore.class,
     RemoteJobPlanner.class})
-public class TestClusterBasedJobCoordinator {

Review comment:
       I'd suggest to keep the old name as is since it is indeed testing the functionality of `ClusterBasedJobCoordinator` like startpoint fanout, partition monitor.
   
   If you can create a new test class for `ClusterBasedJobCoordinatorRunner` and then add test for `runClusterBasedJobCoordinator` (rest of the methods are mostly helpers to construct dependencies) that will be great. 




----------------------------------------------------------------
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