siddhantsangwan commented on code in PR #4614:
URL: https://github.com/apache/ozone/pull/4614#discussion_r1179994098


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java:
##########
@@ -248,17 +434,34 @@ public DatanodeDetails chooseNode(List<DatanodeDetails> 
healthyNodes) {
    * @param excludedNodes - list of the datanodes to excluded. Can be null.
    * @param affinityNodes - the chosen nodes should be on the same rack as
    *                    affinityNodes. Can be null.
+   * @param usedNodes - the chosen nodes should be on the different rack
+   *                    than usedNodes rack when affinityNode is null.
    * @param dataSizeRequired - size required for the container.
    * @param metadataSizeRequired - size required for Ratis metadata.
    * @return List of chosen datanodes.
    * @throws SCMException  SCMException
    */
   private DatanodeDetails chooseNode(List<DatanodeDetails> excludedNodes,
-      List<DatanodeDetails> affinityNodes, long metadataSizeRequired,
+      List<DatanodeDetails> affinityNodes, List<DatanodeDetails> usedNodes,
+      long metadataSizeRequired,
       long dataSizeRequired) throws SCMException {
     int ancestorGen = RACK_LEVEL;
     int maxRetry = MAX_RETRY;
     List<String> excludedNodesForCapacity = null;
+
+    // When affinity node is null, in this case new node to be selected
+    // should be in different rack than used nodes rack.
+    // Exclude nodes should be just excluded from topology node selection,
+    // which is filled in excludedNodesForCapacity
+    // Used node rack should not be part of rack selection
+    // which is filled in excludedNodes

Review Comment:
   From the doc of getLeaf(...), I think `excludedScopes` does relate to rack. 
It says `excludedScope = /dc2/rack2`:
   ```
      *   Input:
      *   leafIndex = 2
      *   excludedScope = /dc2/rack2
      *   excludedNodes = {/dc1/rack1/n1}
      *   ancestorGen = 1
   ```
    I think the scope can be an inner node (data centre, rack) or a leaf node 
(datanode). This PR sets the `excludedScope` as a list of datanodes in 
`chooseNode`, so that looks okay to me.
   



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to