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:
[email protected]
