swamirishi commented on code in PR #4539:
URL: https://github.com/apache/ozone/pull/4539#discussion_r1162363156
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -438,8 +438,7 @@ protected int getRequiredRackCount(int numReplicas) {
if (networkTopology == null) {
return 1;
}
- int maxLevel = networkTopology.getMaxLevel();
- int numRacks = networkTopology.getNumOfNodes(maxLevel - 1);
+ int numRacks = networkTopology.getRackCount();
Review Comment:
I guess we can move this particular logic into SCM common placement policy
instead of relying on NetworkTopology since NetworkTopology class is meant to
be more generic & need not understand racks.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopology.java:
##########
@@ -242,4 +242,10 @@ Node getNode(int leafIndex, String scope, List<String>
excludedScopes,
*/
List<? extends Node> sortByDistanceCost(Node reader,
List<? extends Node> nodes, int activeLen);
+
+ default int getRackCount() {
Review Comment:
Should we have the concept of Racks in Network Topology? Should this
particular function go in PlacementPolicy class instead.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopology.java:
##########
@@ -242,4 +242,10 @@ Node getNode(int leafIndex, String scope, List<String>
excludedScopes,
*/
List<? extends Node> sortByDistanceCost(Node reader,
List<? extends Node> nodes, int activeLen);
+
+ default int getRackCount() {
Review Comment:
@sodonnel I remember us discussing this before.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java:
##########
@@ -386,6 +386,7 @@ protected int getMaxReplicasPerRack(int numReplicas, int
numberOfRacks) {
@Override
protected int getRequiredRackCount(int numReplicas) {
- return REQUIRED_RACKS;
+ int racks = networkTopology != null ? networkTopology.getRackCount() : 1;
+ return Math.min(REQUIRED_RACKS, racks);
Review Comment:
From my understanding, this is the only change which fixes the particular
issue. I think it would be better to create another refactoring jira, if you
want to change the other interfaces.
--
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-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
