[GitHub] [samza] lakshmi-manasa-g commented on a change in pull request #1370: SAMZA-2537: Fix JobCoordinatorLaunchUtil's execution sequence.

[GitBox <gi...@apache.org>]

lakshmi-manasa-g commented on a change in pull request #1370:
URL: https://github.com/apache/samza/pull/1370#discussion_r433533324



##########
File path: samza-core/src/main/java/org/apache/samza/clustermanager/JobCoordinatorLaunchUtil.java
##########
@@ -59,6 +59,8 @@ public static void run(SamzaApplication app, Config config) {
     }
 
     Config fullConfig = jobConfigs.get(0);
+    // Create coordinator stream if does not exist before fetching launch config from it.
+    CoordinatorStreamUtil.createCoordinatorStream(fullConfig);

Review comment:
       From what i understand,  CoordinatorStreamUtil.createCoordinateStream() is idempotent right.. so calling this twice should not have any side effects. Asking since you are not removing the creation from later code paths but adding it here.. so want to be sure.

##########
File path: samza-core/src/main/scala/org/apache/samza/util/CoordinatorStreamUtil.scala
##########
@@ -48,6 +48,23 @@ object CoordinatorStreamUtil extends Logging {
     new MapConfig(coordinatorSystemConfig)
   }
 
+  /**
+   * Creates coordinator stream from config.

Review comment:
       maybe we need to move the comment inside` // ..if it doesnt exist` to the java doc of this method. I feel that would declare better the nature of this method.
   




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