sodonnel commented on a change in pull request #881:
URL: https://github.com/apache/hadoop-ozone/pull/881#discussion_r420683164
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -198,4 +211,69 @@ 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 topology 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) {
+ if (dns.size() > 0) {
+ // placement is always satisfied if there is at least one DN.
+ return validPlacement;
+ } else {
+ return invalidPlacement;
+ }
+ }
Review comment:
I have reworded HDDS-3079 and added a specific note:
> Note that as part of this move, the rack specific logic in
SCMCommonPlacementPolicy.validateContainerPlacement() should be extracted and
moved into this common parent too.
I really don't like the idea of duplicating more code into both classes and
I think the above Jira now makes it clear what should happen when the rack
specific logic moved.
I don't think it is a trivial task to extract the rack specific part of both
placement policies either, so I think it is best left to that other change
rather than doing it with this change. It will make this change too big.
----------------------------------------------------------------
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]
