mynameborat commented on a change in pull request #1446:
URL: https://github.com/apache/samza/pull/1446#discussion_r543405715
##########
File path:
samza-core/src/main/java/org/apache/samza/clustermanager/StandbyContainerManager.java
##########
@@ -375,24 +417,38 @@ boolean checkStandbyConstraints(String
containerIdToStart, String host) {
SamzaResource resource =
samzaApplicationState.pendingProcessors.get(containerID);
// return false if a conflicting container is pending for launch on the
host
- if (resource != null && resource.getHost().equals(host)) {
- log.info("Container {} cannot be started on host {} because container
{} is already scheduled on this host",
- containerIdToStart, host, containerID);
+ if (!checkStandbyConstraintsHelper(containerIdToStart, host, resource,
containerID)) {
return false;
}
// return false if a conflicting container is running on the host
resource = samzaApplicationState.runningProcessors.get(containerID);
- if (resource != null && resource.getHost().equals(host)) {
- log.info("Container {} cannot be started on host {} because container
{} is already running on this host",
- containerIdToStart, host, containerID);
+ if (!checkStandbyConstraintsHelper(containerIdToStart, host, resource,
containerID)) {
return false;
}
}
return true;
}
+ boolean checkStandbyConstraintsHelper(String containerIdToStart, String
hostToStartContainerOn, SamzaResource existingResource, String
existingContainerID) {
Review comment:
Should we guard this with the configuration as well?
##########
File path: samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java
##########
@@ -49,6 +49,11 @@
*/
public static final String CONTAINER_LABEL = "yarn.container.label";
+ /**
+ * Determines whether standby allocation is rack aware or not.
+ */
+ public static final String RACK_AWARE_STANDBY_ENABLED =
"yarn.rack-aware.standby.enabled";
+
Review comment:
What are your thoughts on repurposing this configuration under
`ClusterManagerConfig` something like
`cluster-manager.fault-domain-aware.standby.enabled`?
Here are some of the reasons why i think it will be useful
1. We will need a kill switch in samza core to ensure we can turn off the
usage of `FaultDomainManager`.
2. Current implementation forces `FaultDomainManager` is present regardless
of its usage in the code path and does work even if this flag is turned off
resulting in half-baked experience and unnecessary work.
3. While specific implementation can have cut off switches, implementations
can make consistent assumptions that the information necessary for rack-aware
a.k.a fault domain aware requests will be provided by the samza core as part of
SamzaResourceRequest if its enabled in the core.
4. Provides room for optimizations where the request constraints aren't used
for the scenarios where its disabled at the core but still turned on at the
cluster level.
----------------------------------------------------------------
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]
