You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/04/30 22:14:59 UTC
[GitHub] [hadoop-ozone] avijayanhwx commented on a change in pull request #881: HDDS-3081 Replication manager should detect and correct containers which don't meet the replication policy
avijayanhwx commented on a change in pull request #881:
URL: https://github.com/apache/hadoop-ozone/pull/881#discussion_r418295719
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -198,4 +200,65 @@ public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
*/
public abstract DatanodeDetails chooseNode(
List<DatanodeDetails> healthyNodes);
+
+ /**
+ * Default implementation for basic placement policies that do not have a
+ * placement policy. If the policy has not network toplogy this method should
Review comment:
nit. toplogy -> topology
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -198,4 +200,65 @@ public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
*/
public abstract DatanodeDetails chooseNode(
List<DatanodeDetails> healthyNodes);
+
+ /**
+ * Default implementation for basic placement policies that do not have a
+ * placement policy. If the policy has not network toplogy this method should
+ * return null.
+ * @return The networkTopology for the policy or null if none is configured.
+ */
+ protected NetworkTopology getNetworkTopology() {
+ return null;
+ }
+
+ /**
+ * Default implementation to return the number of racks containers should span
+ * to meet the placement policy. For simple policies that are not rack aware
+ * we return 1, from this default implementation.
+ * should have
+ * @return The number of racks containers should span to meet the policy
+ */
+ protected int getRequiredRackCount() {
+ return 1;
+ }
+
+ /**
+ * This default implementation handles rack aware policies and non rack
+ * aware policies. If a future placement policy needs to check more than racks
+ * to validate the policy (eg node groups, HDFS like upgrade domain) this
+ * method should be overridden in the sub class.
+ * This method requires that subclasses which implement rack aware policies
+ * override the default method getRequiredRackCount and getNetworkTopology.
+ * @param dns List of datanodes holding a replica of the container
+ * @param replicas The expected number of replicas
+ * @return ContainerPlacementStatus indicating if the placement policy is
+ * met or not. Not this only considers the rack count and not the
+ * number of replicas.
+ */
+ @Override
+ public ContainerPlacementStatus validateContainerPlacement(
+ List<DatanodeDetails> dns, int replicas) {
+ NetworkTopology topology = getNetworkTopology();
+ int requiredRacks = getRequiredRackCount();
+ if (topology == null || replicas == 1 || requiredRacks == 1) {
+ // placement is always satisfied if there is at least one DN.
+ return new ContainerPlacementStatusDefault(dns.size(), 1, 1);
+ }
+ // We have a network topology so calculate if it is satisfied or not.
+ int numRacks = 1;
+ final int maxLevel = topology.getMaxLevel();
+ // The leaf nodes are all at max level, so the number of nodes at
+ // leafLevel - 1 is the rack count
+ numRacks = topology.getNumOfNodes(maxLevel- 1);
Review comment:
nit. space after 'maxLevel'.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -198,4 +200,65 @@ public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
*/
public abstract DatanodeDetails chooseNode(
List<DatanodeDetails> healthyNodes);
+
+ /**
+ * Default implementation for basic placement policies that do not have a
+ * placement policy. If the policy has not network toplogy this method should
+ * return null.
+ * @return The networkTopology for the policy or null if none is configured.
+ */
+ protected NetworkTopology getNetworkTopology() {
+ return null;
+ }
+
+ /**
+ * Default implementation to return the number of racks containers should span
+ * to meet the placement policy. For simple policies that are not rack aware
+ * we return 1, from this default implementation.
+ * should have
+ * @return The number of racks containers should span to meet the policy
+ */
+ protected int getRequiredRackCount() {
+ return 1;
+ }
+
+ /**
+ * This default implementation handles rack aware policies and non rack
+ * aware policies. If a future placement policy needs to check more than racks
+ * to validate the policy (eg node groups, HDFS like upgrade domain) this
+ * method should be overridden in the sub class.
+ * This method requires that subclasses which implement rack aware policies
+ * override the default method getRequiredRackCount and getNetworkTopology.
+ * @param dns List of datanodes holding a replica of the container
+ * @param replicas The expected number of replicas
+ * @return ContainerPlacementStatus indicating if the placement policy is
+ * met or not. Not this only considers the rack count and not the
+ * number of replicas.
+ */
+ @Override
+ public ContainerPlacementStatus validateContainerPlacement(
+ List<DatanodeDetails> dns, int replicas) {
+ NetworkTopology topology = getNetworkTopology();
+ int requiredRacks = getRequiredRackCount();
+ if (topology == null || replicas == 1 || requiredRacks == 1) {
+ // placement is always satisfied if there is at least one DN.
+ return new ContainerPlacementStatusDefault(dns.size(), 1, 1);
Review comment:
At this point if we know that the placement is satisfied, do we need to create a class that has to "compute" that again? Can we use an inline object here?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -512,25 +523,61 @@ private void handleUnderReplicatedContainer(final ContainerInfo container,
if (source.size() > 0) {
final int replicationFactor = container
.getReplicationFactor().getNumber();
- final int delta = replicationFactor - getReplicaCount(id, replicas);
+ // Want to check if the container is mis-replicated after considering
+ // inflight add and delete.
+ // Create a new list from source (healthy replicas minus pending delete)
+ List<DatanodeDetails> targetReplicas = new ArrayList<>();
+ targetReplicas.addAll(source);
+ // Then add any pending additions
+ targetReplicas.addAll(replicationInFlight);
+
+ int delta = replicationFactor - getReplicaCount(id, replicas);
+ final int additionalRacks
Review comment:
If delta > 0, do we still need to validate the placement status? Cannot we just choose a DN in accordance to policy and issue the replication command? In that case, this logic can be executed only if we are "misreplicated" and not "underreplicated".
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -512,25 +523,61 @@ private void handleUnderReplicatedContainer(final ContainerInfo container,
if (source.size() > 0) {
final int replicationFactor = container
.getReplicationFactor().getNumber();
- final int delta = replicationFactor - getReplicaCount(id, replicas);
+ // Want to check if the container is mis-replicated after considering
+ // inflight add and delete.
+ // Create a new list from source (healthy replicas minus pending delete)
+ List<DatanodeDetails> targetReplicas = new ArrayList<>();
Review comment:
nit. Can be new ArrayList<>(source).
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -582,17 +629,71 @@ private void handleOverReplicatedContainer(final ContainerInfo container,
.filter(r -> !compareState(container.getState(), r.getState()))
.collect(Collectors.toList());
- //Move the unhealthy replicas to the front of eligible replicas to delete
- eligibleReplicas.removeAll(unhealthyReplicas);
- eligibleReplicas.addAll(0, unhealthyReplicas);
-
- for (int i = 0; i < excess; i++) {
- sendDeleteCommand(container,
- eligibleReplicas.get(i).getDatanodeDetails(), true);
+ // If there are unhealthy replicas, then we should remove them even if it
+ // makes the container violate the placement policy, as excess unhealthy
+ // containers are not really useful. It will be corrected later as a
+ // mis-replicated container will be seen as under-replicated.
+ for (ContainerReplica r : unhealthyReplicas) {
Review comment:
Can we use something like findAny() here instead of breaking from the loop after first iteration? 'excess' is always > 0 here.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -198,4 +200,65 @@ public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
*/
public abstract DatanodeDetails chooseNode(
List<DatanodeDetails> healthyNodes);
+
+ /**
+ * Default implementation for basic placement policies that do not have a
+ * placement policy. If the policy has not network toplogy this method should
+ * return null.
+ * @return The networkTopology for the policy or null if none is configured.
+ */
+ protected NetworkTopology getNetworkTopology() {
+ return null;
+ }
+
+ /**
+ * Default implementation to return the number of racks containers should span
+ * to meet the placement policy. For simple policies that are not rack aware
+ * we return 1, from this default implementation.
+ * should have
+ * @return The number of racks containers should span to meet the policy
+ */
+ protected int getRequiredRackCount() {
+ return 1;
+ }
+
+ /**
+ * This default implementation handles rack aware policies and non rack
+ * aware policies. If a future placement policy needs to check more than racks
+ * to validate the policy (eg node groups, HDFS like upgrade domain) this
+ * method should be overridden in the sub class.
+ * This method requires that subclasses which implement rack aware policies
+ * override the default method getRequiredRackCount and getNetworkTopology.
+ * @param dns List of datanodes holding a replica of the container
+ * @param replicas The expected number of replicas
+ * @return ContainerPlacementStatus indicating if the placement policy is
+ * met or not. Not this only considers the rack count and not the
+ * number of replicas.
+ */
+ @Override
+ public ContainerPlacementStatus validateContainerPlacement(
+ List<DatanodeDetails> dns, int replicas) {
+ NetworkTopology topology = getNetworkTopology();
+ int requiredRacks = getRequiredRackCount();
+ if (topology == null || replicas == 1 || requiredRacks == 1) {
+ // placement is always satisfied if there is at least one DN.
+ return new ContainerPlacementStatusDefault(dns.size(), 1, 1);
+ }
+ // We have a network topology so calculate if it is satisfied or not.
+ int numRacks = 1;
+ final int maxLevel = topology.getMaxLevel();
+ // The leaf nodes are all at max level, so the number of nodes at
+ // leafLevel - 1 is the rack count
+ numRacks = topology.getNumOfNodes(maxLevel- 1);
Review comment:
Should we be doing this getNumOfNodes logN operation for every container in a run of the Replication Manager? Doesn't this stay fairly static?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org