symious commented on code in PR #6050:
URL: https://github.com/apache/ozone/pull/6050#discussion_r1461528437
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -88,19 +93,25 @@ private boolean
isContainerReplicatingOrDeleting(ContainerID containerID) {
* 5. Container should be closed.
* 6. If the {@link LegacyReplicationManager} is enabled, then the container
should not be an EC container.
* @param node DatanodeDetails for which to find candidate containers.
- * @return NavigableSet of candidate containers that satisfy the criteria.
+ * @return Set of candidate containers that satisfy the criteria.
*/
- public NavigableSet<ContainerID> getCandidateContainers(
+ public Set<ContainerID> getCandidateContainers(
DatanodeDetails node, long sizeMovedAlready) {
- NavigableSet<ContainerID> containerIDSet =
- new TreeSet<>(orderContainersByUsedBytes().reversed());
try {
- containerIDSet.addAll(nodeManager.getContainers(node));
+ // Initialize containerSet for node
+ if (!setMap.containsKey(node)) {
+ addNodeToSetMap(node);
+ }
+ // In case the node is removed
+ nodeManager.getContainers(node);
} catch (NodeNotFoundException e) {
LOG.warn("Could not find Datanode {} while selecting candidate " +
"containers for Container Balancer.", node.toString(), e);
- return containerIDSet;
+ setMap.remove(node);
+ return Collections.emptySet();
Review Comment:
> can return Collections.emptySet()
@sumitagrawl Updated.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -242,4 +253,12 @@ public void setSelectedContainers(
this.selectedContainers = selectedContainers;
}
+
+ private void addNodeToSetMap(DatanodeDetails node)
Review Comment:
> ContaienrBalancerSelectionCriteria is applicable within one iteration
>
> * all filtering can be done before setting to setMap
> * Next filtering can be basic including selectedContainers and
shouldBeExcluded
>
> Suggested to use getCandidateContainers() to update setMap wrapping in
another method.
@sumitagrawl Wrapped the method here.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -242,4 +253,12 @@ public void setSelectedContainers(
this.selectedContainers = selectedContainers;
}
+
+ private void addNodeToSetMap(DatanodeDetails node)
+ throws NodeNotFoundException {
+ NavigableSet<ContainerID> newSet =
+ new TreeSet<>(orderContainersByUsedBytes().reversed());
+ newSet.addAll(nodeManager.getContainers(node));
Review Comment:
> IMO, sorting can be done on filtered containers only to avoid sorting of
containers not meeting criteria.
@sumitagrawl Updated.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -88,19 +93,25 @@ private boolean
isContainerReplicatingOrDeleting(ContainerID containerID) {
* 5. Container should be closed.
* 6. If the {@link LegacyReplicationManager} is enabled, then the container
should not be an EC container.
* @param node DatanodeDetails for which to find candidate containers.
- * @return NavigableSet of candidate containers that satisfy the criteria.
+ * @return Set of candidate containers that satisfy the criteria.
*/
- public NavigableSet<ContainerID> getCandidateContainers(
+ public Set<ContainerID> getCandidateContainers(
Review Comment:
> can return Set<> and can change NavigableSet<> to Set in usages, but
implementation can refer TreeMap.
@sumitagrawl Updated.
--
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]
