[GitHub] [ozone] sodonnel commented on a diff in pull request #5320: HDDS-9315. Fix comparison logic for SCMContainerPlacementCapacity.

["sodonnel (via GitHub)" <gi...@apache.org>]

sodonnel commented on code in PR #5320:
URL: https://github.com/apache/ozone/pull/5320#discussion_r1340351900


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/metrics/SCMNodeMetric.java:
##########
@@ -105,8 +105,8 @@ public boolean isLess(SCMNodeStat o) {
     }
 
     // if these nodes are have similar weight then return the node with less
-    // free space as the lesser node.
-    return stat.getRemaining().isLess(o.getRemaining().get());
+    // used space as the lesser node.
+    return stat.getScmUsed().isLess(o.getScmUsed().get());

Review Comment:
   Say we have 2 disks, one has a capacity of 100, with 20 used and one has a capacity of 50 with 10 used. So the used / capacity is 0.2 for both.
   
   Remaining on disk 1 is 80.
   
   Remaining on disk 2 is 40.
   
   The original code would have returned 80.isGreater(40) -> 80 wins, which is probably not correct as we are looking for the "more used node" and the first disk actually has more free space.
   
   The new code will return 10.isGreater(20) -> the other disk wins, which is the one with the least free space.
   
   Therefore I believe this change is correct. It would also have probably been correct to change the original code to:
   
   ```
   stat.getRemaining().isGreater(o.getRemaining().get());
   ```



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org