Skip to content

HDDS-10160. Cache sort results in ContainerBalancerSelectionCriteria #6050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 11 commits into from
Jan 30, 2024
Merged

HDDS-10160. Cache sort results in ContainerBalancerSelectionCriteria #6050

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
63b257a
HDDS-10160. Cache sort results in ContainerBalancerSelectionCriteria
symious Jan 19, 2024
e18496a
HDDS-10160. Sort out the code
symious Jan 22, 2024
968cde7
HDDS-10160. Filter selected and excluded containers
symious Jan 22, 2024
2f4ec85
HDDS-10160. check on iteration
symious Jan 23, 2024
05c5a05
Remove containers from candidationContainerSet
symious Jan 23, 2024
54d336f
Remove containerId from candidateContainerSet
symious Jan 23, 2024
7f3b085
HDDS-10160. Add comment
symious Jan 24, 2024
92c7ba0
HDDS-10160. Use isNodeRegistered
symious Jan 26, 2024
7b590d2
HDDS-10160. Implement MockNodeManager's isNodeRegistered()
symious Jan 26, 2024
b2c3bcb
HDDS-10160. Code cleaning
symious Jan 26, 2024
8c00ae7
HDDS-10160. Update comments
symious Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions ...ava/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.NavigableSet;
import java.util.Set;
import java.util.TreeSet;
Expand All @@ -52,6 +55,7 @@ public class ContainerBalancerSelectionCriteria {
private Set<ContainerID> selectedContainers;
private Set<ContainerID> excludeContainers;
private FindSourceStrategy findSourceStrategy;
private Map<DatanodeDetails, NavigableSet<ContainerID>> setMap;

public ContainerBalancerSelectionCriteria(
ContainerBalancerConfiguration balancerConfiguration,
Expand All @@ -66,6 +70,7 @@ public ContainerBalancerSelectionCriteria(
selectedContainers = new HashSet<>();
excludeContainers = balancerConfiguration.getExcludeContainers();
this.findSourceStrategy = findSourceStrategy;
this.setMap = new HashMap<>();
}

/**
Expand All @@ -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(
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
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)) {
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
addNodeToSetMap(node);
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
}
// In case the node is removed
nodeManager.getContainers(node);

@siddhantsangwan siddhantsangwan Jan 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we making this call just to see if an exception gets thrown? In that case this is a bit awkward and confusing. Does node manager provide an API that we can use first to check if SCM knows the node, and then get its containers (or remove them from the cache is the node isn't there anymore)?

@symious symious Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, since currently there is no explicit method to check if a node exists, this is used for this check.

@sodonnel sodonnel Jan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe getNodeStatus would be a better one to check the node is still there? getcontainers creates a new HashSet of all the containers on the node, so it is somewhat expensive if we don't use those returned contaienrs.

@adoroszlai adoroszlai Jan 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @sodonnel. Would isNodeRegistered be even better?

ozone/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java

Lines 627 to 634 in ac597ad

@Override
public Boolean isNodeRegistered(DatanodeDetails datanodeDetails) {
try {
nodeStateManager.getNode(datanodeDetails);
return true;
} catch (NodeNotFoundException e) {
return false;
}

} 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();
Comment thread
sumitagrawl marked this conversation as resolved.
}

NavigableSet<ContainerID> containerIDSet = setMap.get(node);
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
if (excludeContainers != null) {
containerIDSet.removeAll(excludeContainers);
}
Expand Down Expand Up @@ -242,4 +253,12 @@ public void setSelectedContainers(
this.selectedContainers = selectedContainers;
}


private void addNodeToSetMap(DatanodeDetails node)
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
throws NodeNotFoundException {
NavigableSet<ContainerID> newSet =
new TreeSet<>(orderContainersByUsedBytes().reversed());
newSet.addAll(nodeManager.getContainers(node));
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
setMap.put(node, newSet);
}
}
3 changes: 1 addition & 2 deletions ...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.NavigableSet;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -692,7 +691,7 @@ private long cancelMovesThatExceedTimeoutDuration() {
* @return ContainerMoveSelection containing the selected target and container
*/
private ContainerMoveSelection matchSourceWithTarget(DatanodeDetails source) {
NavigableSet<ContainerID> candidateContainers =
Set<ContainerID> candidateContainers =
selectionCriteria.getCandidateContainers(source,
sizeScheduledForMoveInLatestIteration);

Expand Down