[GitHub] [samza] mynameborat 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>]

mynameborat commented on a change in pull request #1439:
URL: https://github.com/apache/samza/pull/1439#discussion_r524347041



##########
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:
       I would suggest extracting the format as a constant and hardcoding some of the format constants instead of doing it the other way.
   e.g. 
   ```
     CONTAINER_HEART_BEAT_SERVLET_FORMAT = "%s" + "/containerHeartbeat";
     CONTAINER_EXECUTION_ID_PARAM_FORMAT = "executionContainerId=" + "%s";
     CONTAINER_HEART_BEAT_ENDPOINT_FORMAT = CONTAINER_HEART_BEAT_SERVLET_FORMAT + CONTAINER_EXECUTION_ID_PARAM_FORMAT
   ```
   
   Or some flavor of the above, so that you can reuse the format across the unit tests & other places in code. IMO, having the constants by themselves still forces users to figure out how they come together and repetitive code e.g. `"%s%s?%s=%s"` with readability hit as well.

##########
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:
       I'd also suggest moving this to Yarn specific constants file and remove the `ClusterBased` prefix as these don't have any implications outside the YARN world as they are all very specific to how YARN operates and aren't used in general samza core orchestration.
   
   [1] `samza.autoscaling.server.url` should have gotten cleaned up when we removed the auto-scaling module. We can take this opportunity and rename this configuration to `am.tracking.url` or something else as this is unused.
   [2] If we want to keep it the way it is for backward compatibility reasons, we should add a comment on how this is being used. 
   
   I am in favor of [1]. wdyt?




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