You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/09/09 15:11:05 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
swamirishi opened a new pull request, #3744:
URL: https://github.com/apache/ozone/pull/3744
## What changes were proposed in this pull request?
The existing interface only allows for excluded nodes to be passed, which we use to exclude nodes already used in a pipeline. However when we go to add a new node to a pipeline (eg EC Reconstruction, or replication) the excluded node list cannot be used to indicate the existing racks used. This means the placement policies are not "rack aware" when used to select additional nodes. We will pick new nodes, and it may cause the container to become mis-replicated (ie not on enough racks).
Ideally we should pass in the existing nodes separately from excluded nodes. That would allow the policies to lookup the racks of those nodes and hence correctly select new nodes that meet the placement policy.
Changing the interface to segregate usedNodes & excludedNodes(Nodes with Failure)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7206
## How was this patch tested?
Unit Test Cases
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r974545192
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -76,85 +77,22 @@ public SCMContainerPlacementRackScatter(final NodeManager nodeManager,
this.metrics = metrics;
}
- /**
- * Called by SCM to choose datanodes.
- *
- *
- * @param excludedNodes - list of the datanodes to exclude.
- * @param favoredNodes - list of nodes preferred. This is a hint to the
- * allocator, whether the favored nodes will be used
- * depends on whether the nodes meets the allocator's
- * requirement.
- * @param nodesRequiredToChoose - number of datanodes required.
- * @param dataSizeRequired - size required for the container.
- * @param metadataSizeRequired - size required for Ratis metadata.
- * @return List of datanodes.
- * @throws SCMException SCMException
- */
- @Override
- protected List<DatanodeDetails> chooseDatanodesInternal(
- final List<DatanodeDetails> excludedNodes,
- final List<DatanodeDetails> favoredNodes,
- final int nodesRequiredToChoose, final long metadataSizeRequired,
- final long dataSizeRequired) throws SCMException {
- if (nodesRequiredToChoose <= 0) {
- String errorMsg = "num of nodes required to choose should bigger" +
- "than 0, but the given num is " + nodesRequiredToChoose;
- throw new SCMException(errorMsg, null);
- }
- metrics.incrDatanodeRequestCount(nodesRequiredToChoose);
- int nodesRequired = nodesRequiredToChoose;
- int excludedNodesCount = excludedNodes == null ? 0 : excludedNodes.size();
- List<Node> availableNodes = networkTopology.getNodes(
- networkTopology.getMaxLevel());
- int totalNodesCount = availableNodes.size();
- if (excludedNodes != null) {
- availableNodes.removeAll(excludedNodes);
+ public Set<DatanodeDetails> chooseNodesFromRacks(List<Node> racks,
Review Comment:
It is same code and moved to a different function. Previously the function was very long.
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r970968583
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java:
##########
@@ -98,9 +99,11 @@ public SCMContainerPlacementRackAware(final NodeManager nodeManager,
*/
@Override
protected List<DatanodeDetails> chooseDatanodesInternal(
- List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
- int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
- throws SCMException {
+ List<DatanodeDetails> usedNodes,
Review Comment:
We don't use excluded nodes anywhere in this method - is that intentional, ie something we are going to address later, or does it never need to be used here?
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r971070115
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -220,25 +158,171 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
// If chosenNodes changed, reset the retryCount
retryCount = 0;
}
+ maxOuterLoopIterations--;
}
- List<DatanodeDetails> result = new ArrayList<>(chosenNodes);
- ContainerPlacementStatus placementStatus =
- validateContainerPlacement(result, nodesRequiredToChoose);
- if (!placementStatus.isPolicySatisfied()) {
- String errorMsg = "ContainerPlacementPolicy not met, currentRacks is " +
- placementStatus.actualPlacementCount() + "desired racks is" +
- placementStatus.expectedPlacementCount();
+ return chosenNodes;
+ }
+
+ /**
+ * Called by SCM to choose datanodes.
+ *
+ * @param usedNodes - list of the datanodes to already chosen in the
+ * pipeline.
+ * @param excludedNodes - list of the datanodes to exclude.
+ * @param favoredNodes - list of nodes preferred. This is a hint to the
+ * allocator, whether the favored nodes will be used
+ * depends on whether the nodes meets the allocator's
+ * requirement.
+ * @param nodesRequiredToChoose - number of datanodes required.
+ * @param dataSizeRequired - size required for the container.
+ * @param metadataSizeRequired - size required for Ratis metadata.
+ * @return List of datanodes.
+ * @throws SCMException SCMException
+ */
+ @Override
+ protected List<DatanodeDetails> chooseDatanodesInternal(
+ List<DatanodeDetails> usedNodes,
+ final List<DatanodeDetails> excludedNodes,
+ final List<DatanodeDetails> favoredNodes,
+ final int nodesRequiredToChoose, final long metadataSizeRequired,
+ final long dataSizeRequired) throws SCMException {
+ if (nodesRequiredToChoose <= 0) {
+ String errorMsg = "num of nodes required to choose should bigger" +
+ "than 0, but the given num is " + nodesRequiredToChoose;
throw new SCMException(errorMsg, null);
}
+ metrics.incrDatanodeRequestCount(nodesRequiredToChoose);
+ int nodesRequired = nodesRequiredToChoose;
+ int excludedNodesCount = excludedNodes == null ? 0 : excludedNodes.size();
+ List<Node> availableNodes = networkTopology.getNodes(
+ networkTopology.getMaxLevel());
+ int totalNodesCount = availableNodes.size();
+ if (excludedNodes != null) {
+ availableNodes.removeAll(excludedNodes);
+ }
+ if (availableNodes.size() < nodesRequired) {
+ throw new SCMException("No enough datanodes to choose. " +
+ "TotalNode = " + totalNodesCount +
+ " AvailableNode = " + availableNodes.size() +
+ " RequiredNode = " + nodesRequired +
+ " ExcludedNode = " + excludedNodesCount,
+ SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE);
+ }
+
+ List<DatanodeDetails> mutableFavoredNodes = new ArrayList<>();
+ if (favoredNodes != null) {
+ // Generate mutableFavoredNodes, only stores valid favoredNodes
+ for (DatanodeDetails datanodeDetails : favoredNodes) {
+ if (isValidNode(datanodeDetails, metadataSizeRequired,
+ dataSizeRequired)) {
+ mutableFavoredNodes.add(datanodeDetails);
+ }
+ }
+ Collections.shuffle(mutableFavoredNodes);
+ }
+ if (excludedNodes != null) {
+ mutableFavoredNodes.removeAll(excludedNodes);
+ }
+
+ if (usedNodes == null) {
+ usedNodes = Collections.emptyList();
+ }
+ List<Node> racks = getAllRacks();
+ Set<Node> usedRacks = usedNodes.stream()
+ .map(node -> networkTopology.getAncestor(node, RACK_LEVEL))
+ .filter(node -> node != null)
+ .collect(Collectors.toSet());
+ int requiredReplicationFactor = usedNodes.size() + nodesRequired;
+ int numberOfRacksRequired =
+ getRequiredRackCount(requiredReplicationFactor);
+ int additionalRacksRequired = numberOfRacksRequired - usedRacks.size();
+ if (nodesRequired < additionalRacksRequired) {
+ String reason = "Required nodes size: " + nodesRequired
+ + " is less than required number of racks to choose: "
+ + additionalRacksRequired + ".";
+ LOG.warn("Placement policy cannot choose the enough racks. {}"
+ + "Total number of Required Racks: {} Used Racks Count:" +
+ " {}, Required Nodes count: {}",
+ reason, numberOfRacksRequired, usedRacks.size(), nodesRequired);
+ throw new SCMException(reason,
Review Comment:
There are a few new `throw new SCMException(reason,` here I think - are we making this class more strict, in that if it cannot spread the nodes across racks it will throw, or are we keeping it largely as it was?
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r971025496
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java:
##########
@@ -98,9 +99,11 @@ public SCMContainerPlacementRackAware(final NodeManager nodeManager,
*/
@Override
protected List<DatanodeDetails> chooseDatanodesInternal(
- List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
- int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
- throws SCMException {
+ List<DatanodeDetails> usedNodes,
Review Comment:
Currently implementation has been done only for RackScatterPolicy. We have to change logic for other implementations as well.
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r971035099
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java:
##########
@@ -98,9 +99,11 @@ public SCMContainerPlacementRackAware(final NodeManager nodeManager,
*/
@Override
protected List<DatanodeDetails> chooseDatanodesInternal(
- List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
- int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
- throws SCMException {
+ List<DatanodeDetails> usedNodes,
Review Comment:
ok - makes sense.
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r970958615
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:
##########
@@ -146,8 +159,14 @@ object of DatanodeDetails(with Topology Information) while trying to get the
random node from NetworkTopology should fix this. Check HDDS-7015
*/
return chooseDatanodesInternal(
+ Objects.isNull(usedNodes) ? Collections.emptyList()
Review Comment:
Might be a good idea to move the logic for handling the empty collection and mapping it into a private method, as I think its duplicate here for used and excluded nodes?
--
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
[GitHub] [ozone] sodonnel merged pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel merged PR #3744:
URL: https://github.com/apache/ozone/pull/3744
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r974546941
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java:
##########
@@ -215,8 +216,9 @@ private boolean multipleRacksAvailable(List<DatanodeDetails> dns) {
*/
@Override
protected List<DatanodeDetails> chooseDatanodesInternal(
- List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
- int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
+ List<DatanodeDetails> usedNodes, List<DatanodeDetails> excludedNodes,
Review Comment:
Currently this ticket is making changes for Rack scatter policy only.
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r970963707
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java:
##########
@@ -215,8 +216,9 @@ private boolean multipleRacksAvailable(List<DatanodeDetails> dns) {
*/
@Override
protected List<DatanodeDetails> chooseDatanodesInternal(
- List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
- int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
+ List<DatanodeDetails> usedNodes, List<DatanodeDetails> excludedNodes,
Review Comment:
UsedNodes isn't used anywhere in this class, correct? If so, maybe add a comment to the Java doc for that parameter to highlight that.
I also wonder if we should do anything if someone passes something non-empty for used nodes here? Should we merge them into excludedNodes? Or throw an exception of some sort to hightlight the passed value will not actually be used?
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r971068975
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -76,85 +77,22 @@ public SCMContainerPlacementRackScatter(final NodeManager nodeManager,
this.metrics = metrics;
}
- /**
- * Called by SCM to choose datanodes.
- *
- *
- * @param excludedNodes - list of the datanodes to exclude.
- * @param favoredNodes - list of nodes preferred. This is a hint to the
- * allocator, whether the favored nodes will be used
- * depends on whether the nodes meets the allocator's
- * requirement.
- * @param nodesRequiredToChoose - number of datanodes required.
- * @param dataSizeRequired - size required for the container.
- * @param metadataSizeRequired - size required for Ratis metadata.
- * @return List of datanodes.
- * @throws SCMException SCMException
- */
- @Override
- protected List<DatanodeDetails> chooseDatanodesInternal(
- final List<DatanodeDetails> excludedNodes,
- final List<DatanodeDetails> favoredNodes,
- final int nodesRequiredToChoose, final long metadataSizeRequired,
- final long dataSizeRequired) throws SCMException {
- if (nodesRequiredToChoose <= 0) {
- String errorMsg = "num of nodes required to choose should bigger" +
- "than 0, but the given num is " + nodesRequiredToChoose;
- throw new SCMException(errorMsg, null);
- }
- metrics.incrDatanodeRequestCount(nodesRequiredToChoose);
- int nodesRequired = nodesRequiredToChoose;
- int excludedNodesCount = excludedNodes == null ? 0 : excludedNodes.size();
- List<Node> availableNodes = networkTopology.getNodes(
- networkTopology.getMaxLevel());
- int totalNodesCount = availableNodes.size();
- if (excludedNodes != null) {
- availableNodes.removeAll(excludedNodes);
+ public Set<DatanodeDetails> chooseNodesFromRacks(List<Node> racks,
Review Comment:
There is a lot of change in this class - is this method copied and moved from already existing code, or is the logic here new?
--
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
[GitHub] [ozone] sodonnel commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r976665346
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:
##########
@@ -146,8 +159,14 @@ object of DatanodeDetails(with Topology Information) while trying to get the
random node from NetworkTopology should fix this. Check HDDS-7015
*/
return chooseDatanodesInternal(
+ Objects.isNull(usedNodes) ? Collections.emptyList()
Review Comment:
The modified code looks like:
```
return chooseDatanodesInternal(
Objects.isNull(usedNodes) ? Collections.emptyList()
: usedNodes.stream().map(node -> {
DatanodeDetails datanodeDetails =
nodeManager.getNodeByUuid(node.getUuidString());
return datanodeDetails != null ? datanodeDetails : node;
}).collect(Collectors.toList()),
Objects.isNull(excludedNodes)
? excludedNodes : excludedNodes.stream()
? Collections.emptyList() : excludedNodes.stream()
.map(node -> {
DatanodeDetails datanodeDetails =
nodeManager.getNodeByUuid(node.getUuidString());
return datanodeDetails != null ? datanodeDetails : node;
}).collect(Collectors.toList()));
```
Could we not change this to look like:
```
private List<DatanodeDetails> validateDatanodes(List<DatanodeDetails> dns) {
return Objects.isNull(dns) ? Collections.emptyList()
: dns.stream().map(node -> {
DatanodeDetails datanodeDetails =
nodeManager.getNodeByUuid(node.getUuidString());
return datanodeDetails != null ? datanodeDetails : node;
}).collect(Collectors.toList())
}
return chooseDatanodesInternal(
validateDatanodes(usedNodes), validateDatanode(excludedNodes);
_
```
The logic used to map both used and excluded looks to be the same to me, and its complicated enough it should be extracted into a method I think.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r973636212
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:
##########
@@ -146,8 +159,14 @@ object of DatanodeDetails(with Topology Information) while trying to get the
random node from NetworkTopology should fix this. Check HDDS-7015
*/
return chooseDatanodesInternal(
+ Objects.isNull(usedNodes) ? Collections.emptyList()
Review Comment:
@sodonnel Is it not better do the Null Check handling in one place.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #3744: HDDS-7206. Change the placement policy interface to allow existing nodes to be specified for Rack Scatter Policy.
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3744:
URL: https://github.com/apache/ozone/pull/3744#discussion_r974544379
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -220,25 +158,171 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
// If chosenNodes changed, reset the retryCount
retryCount = 0;
}
+ maxOuterLoopIterations--;
}
- List<DatanodeDetails> result = new ArrayList<>(chosenNodes);
- ContainerPlacementStatus placementStatus =
- validateContainerPlacement(result, nodesRequiredToChoose);
- if (!placementStatus.isPolicySatisfied()) {
- String errorMsg = "ContainerPlacementPolicy not met, currentRacks is " +
- placementStatus.actualPlacementCount() + "desired racks is" +
- placementStatus.expectedPlacementCount();
+ return chosenNodes;
+ }
+
+ /**
+ * Called by SCM to choose datanodes.
+ *
+ * @param usedNodes - list of the datanodes to already chosen in the
+ * pipeline.
+ * @param excludedNodes - list of the datanodes to exclude.
+ * @param favoredNodes - list of nodes preferred. This is a hint to the
+ * allocator, whether the favored nodes will be used
+ * depends on whether the nodes meets the allocator's
+ * requirement.
+ * @param nodesRequiredToChoose - number of datanodes required.
+ * @param dataSizeRequired - size required for the container.
+ * @param metadataSizeRequired - size required for Ratis metadata.
+ * @return List of datanodes.
+ * @throws SCMException SCMException
+ */
+ @Override
+ protected List<DatanodeDetails> chooseDatanodesInternal(
+ List<DatanodeDetails> usedNodes,
+ final List<DatanodeDetails> excludedNodes,
+ final List<DatanodeDetails> favoredNodes,
+ final int nodesRequiredToChoose, final long metadataSizeRequired,
+ final long dataSizeRequired) throws SCMException {
+ if (nodesRequiredToChoose <= 0) {
+ String errorMsg = "num of nodes required to choose should bigger" +
+ "than 0, but the given num is " + nodesRequiredToChoose;
throw new SCMException(errorMsg, null);
}
+ metrics.incrDatanodeRequestCount(nodesRequiredToChoose);
+ int nodesRequired = nodesRequiredToChoose;
+ int excludedNodesCount = excludedNodes == null ? 0 : excludedNodes.size();
+ List<Node> availableNodes = networkTopology.getNodes(
+ networkTopology.getMaxLevel());
+ int totalNodesCount = availableNodes.size();
+ if (excludedNodes != null) {
+ availableNodes.removeAll(excludedNodes);
+ }
+ if (availableNodes.size() < nodesRequired) {
+ throw new SCMException("No enough datanodes to choose. " +
+ "TotalNode = " + totalNodesCount +
+ " AvailableNode = " + availableNodes.size() +
+ " RequiredNode = " + nodesRequired +
+ " ExcludedNode = " + excludedNodesCount,
+ SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE);
+ }
+
+ List<DatanodeDetails> mutableFavoredNodes = new ArrayList<>();
+ if (favoredNodes != null) {
+ // Generate mutableFavoredNodes, only stores valid favoredNodes
+ for (DatanodeDetails datanodeDetails : favoredNodes) {
+ if (isValidNode(datanodeDetails, metadataSizeRequired,
+ dataSizeRequired)) {
+ mutableFavoredNodes.add(datanodeDetails);
+ }
+ }
+ Collections.shuffle(mutableFavoredNodes);
+ }
+ if (excludedNodes != null) {
+ mutableFavoredNodes.removeAll(excludedNodes);
+ }
+
+ if (usedNodes == null) {
+ usedNodes = Collections.emptyList();
+ }
+ List<Node> racks = getAllRacks();
+ Set<Node> usedRacks = usedNodes.stream()
+ .map(node -> networkTopology.getAncestor(node, RACK_LEVEL))
+ .filter(node -> node != null)
+ .collect(Collectors.toSet());
+ int requiredReplicationFactor = usedNodes.size() + nodesRequired;
+ int numberOfRacksRequired =
+ getRequiredRackCount(requiredReplicationFactor);
+ int additionalRacksRequired = numberOfRacksRequired - usedRacks.size();
+ if (nodesRequired < additionalRacksRequired) {
+ String reason = "Required nodes size: " + nodesRequired
+ + " is less than required number of racks to choose: "
+ + additionalRacksRequired + ".";
+ LOG.warn("Placement policy cannot choose the enough racks. {}"
+ + "Total number of Required Racks: {} Used Racks Count:" +
+ " {}, Required Nodes count: {}",
+ reason, numberOfRacksRequired, usedRacks.size(), nodesRequired);
+ throw new SCMException(reason,
Review Comment:
We are keeping the logic as it is, that it should spread across as many racks as possible, based on the usedNodes & the nodes selected.
--
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