sodonnel commented on a change in pull request #881:
URL: https://github.com/apache/hadoop-ozone/pull/881#discussion_r418512519
##########
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:
That is a good point. This should be static except when nodes register
or are removed, which is rare.
I don't really want to pass "rack count" into this method, as in the future,
we may have other implementations of this (eg node groups, upgrade domain etc),
and putting that into the interface would not be ideal.
I wonder if it would make sense to cache the count for a given level in
`NetworkTopologyImpl`, rather than passing around the count here? The only
things that change it, are add() and remove(), so we could easily have a hash
map or list to store the count for a given level, and upon any add or remove
wipe the cached values. This would be fairly simple to do and would benefit any
other parts of the code which happen to call this existing method.
Do you think that makes sense?
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
