[GitHub] [samza] lakshmi-manasa-g commented on a change in pull request #1439: SAMZA-2600: [refactor] Create constants for string literals used in AM and container

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

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



##########
File path: samza-core/src/main/java/org/apache/samza/container/ContainerHeartbeatClient.java
##########
@@ -56,7 +57,8 @@
 
   public ContainerHeartbeatClient(String coordinatorUrl, String executionEnvContainerId) {
     this.heartbeatEndpoint =
-        String.format("%scontainerHeartbeat?executionContainerId=%s", coordinatorUrl, executionEnvContainerId);
+        String.format("%s%s?%s=%s", coordinatorUrl, CoordinationConstants.CLUSTERBASED_CONTAINER_HEARTBEAT_SERVELET,

Review comment:
       agree that "%s%s?%s=%s" is not readable. Will split as suggested above.
   however, the point of the PR is to NOT have this hardcoded in both AM and container code separately. 
   having them hardcoded separately in AM code and then have it exactly hardcoded in container code is risky imo
   

##########
File path: samza-core/src/main/java/org/apache/samza/coordinator/CoordinationConstants.java
##########
@@ -27,4 +27,7 @@ private CoordinationConstants() {}
   public static final String APPLICATION_RUNNER_PATH_SUFFIX = "ApplicationRunnerData";
   public static final String RUNID_LOCK_ID = "runId";
   public static final int LOCK_TIMEOUT_MS = 300000;
+  public static final String CLUSTERBASED_CONTAINER_HEARTBEAT_SERVELET = "containerHeartbeat";
+  public static final String CLUSTERBASED_EXECUTION_ENVIRONMENT_CONTAINER_ID = "executionContainerId";
+  public static final String CLUSTERBASED_COORDINATOR_URL = "samza.autoscaling.server.url";

Review comment:
       im in favor of [1] too.
   
   agree about these constants being yarn specific but the components using these components (ex: ContainerHeartbeatClient) are part of samza-core. hence making this a yarn specific file in samza-yarn module will cause circular dependency issues.
   
   Additionally, the existing CoordinationConstants file is not labelled standalone specific - although it has only standalone constants. Hence, felt this was a good place to put all constants needed for coordination code




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