You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sodonnel (via GitHub)" <gi...@apache.org> on 2023/05/10 09:53:04 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
sodonnel opened a new pull request, #4694:
URL: https://github.com/apache/ozone/pull/4694
## What changes were proposed in this pull request?
With [HDDS-7226](https://issues.apache.org/jira/browse/HDDS-7226) merged, the RackAwarePlacementPolicy now supports passing used nodes and excluded nodes separately. This allows it to select the racks correctly when there are one or two used nodes.
We need to change the Ratis under and mis-replication handlers to pass the used nodes and excluded nodes as two separate lists now.
Also the EC under-replication handler should be fixed to pass used nodes separately too.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8504
## How was this patch tested?
New unit and changed tests
--
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 #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4694:
URL: https://github.com/apache/ozone/pull/4694#discussion_r1190244821
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -124,19 +124,15 @@ public int processAndSendCommands(
return 0;
}
- // don't place reconstructed replicas on exclude nodes, since they already
- // have replicas
- List<DatanodeDetails> excludedNodes = replicas.stream()
- .map(ContainerReplica::getDatanodeDetails)
- .collect(Collectors.toList());
- // DNs that are already waiting to receive replicas cannot be targets
- excludedNodes.addAll(
- pendingOps.stream()
- .filter(containerReplicaOp -> containerReplicaOp.getOpType() ==
- ContainerReplicaOp.PendingOpType.ADD)
- .map(ContainerReplicaOp::getTarget)
- .collect(Collectors.toList()));
+ ReplicationManagerUtil.ExcludedAndUsedNodes excludedAndUsedNodes =
+ ReplicationManagerUtil.getExcludedAndUsedNodes(
+ new ArrayList<>(replicas), Collections.emptySet(), pendingOps,
+ replicationManager);
+ List<DatanodeDetails> excludedNodes
+ = excludedAndUsedNodes.getExcludedNodes();
excludedNodes.addAll(replicationManager.getExcludedNodes());
Review Comment:
The reason we only add `replicationManager.getExcludedNodes()` to the excludeList in the EC handler is because of the way the command targets work.
For replication commands, we have the sources, and then select the least loaded one to act as the source for the push replication. If they are all overloaded we give an error, and we don't consider load on the target right now. Hence we don't try to exclude any overloaded nodes as targets, as all our load limiting is based on the number of queued commands. For replication we are push, so we don't have a limit on the target.
For reconstruction, we have to select target nodes, and one of those targets must be the place the command is sent to. Therefore we use the exclude list to avoid selecting already overloaded command targets. However, if there is decommission or maintenance too, the same exclude list applies to all of the EC handler, so decommission and maintenance replicate commands would have their targets restricted by this exclude list too.
It is a valid point - should we avoid using "overloaded with too many commands" as a target for replication? It would be simple to do it, as we could just move that one line of code into that new method. It would also make it consistent with how the EC decommission and maintenance behave. Need to think about it a bit more.
--
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] adoroszlai commented on a diff in pull request #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4694:
URL: https://github.com/apache/ozone/pull/4694#discussion_r1190250169
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -124,19 +124,15 @@ public int processAndSendCommands(
return 0;
}
- // don't place reconstructed replicas on exclude nodes, since they already
- // have replicas
- List<DatanodeDetails> excludedNodes = replicas.stream()
- .map(ContainerReplica::getDatanodeDetails)
- .collect(Collectors.toList());
- // DNs that are already waiting to receive replicas cannot be targets
- excludedNodes.addAll(
- pendingOps.stream()
- .filter(containerReplicaOp -> containerReplicaOp.getOpType() ==
- ContainerReplicaOp.PendingOpType.ADD)
- .map(ContainerReplicaOp::getTarget)
- .collect(Collectors.toList()));
+ ReplicationManagerUtil.ExcludedAndUsedNodes excludedAndUsedNodes =
+ ReplicationManagerUtil.getExcludedAndUsedNodes(
+ new ArrayList<>(replicas), Collections.emptySet(), pendingOps,
+ replicationManager);
+ List<DatanodeDetails> excludedNodes
+ = excludedAndUsedNodes.getExcludedNodes();
excludedNodes.addAll(replicationManager.getExcludedNodes());
Review Comment:
I missed (forgot) those details, thanks for the explanation.
--
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 pull request #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #4694:
URL: https://github.com/apache/ozone/pull/4694#issuecomment-1544221428
I am going to commit this as it is. We can consider in the future if it makes sense to add the replicationManager excluded nodes into the master exclude list for simple replication. Right now its only used for EC under replication.
--
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] adoroszlai commented on a diff in pull request #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4694:
URL: https://github.com/apache/ozone/pull/4694#discussion_r1190197564
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -124,19 +124,15 @@ public int processAndSendCommands(
return 0;
}
- // don't place reconstructed replicas on exclude nodes, since they already
- // have replicas
- List<DatanodeDetails> excludedNodes = replicas.stream()
- .map(ContainerReplica::getDatanodeDetails)
- .collect(Collectors.toList());
- // DNs that are already waiting to receive replicas cannot be targets
- excludedNodes.addAll(
- pendingOps.stream()
- .filter(containerReplicaOp -> containerReplicaOp.getOpType() ==
- ContainerReplicaOp.PendingOpType.ADD)
- .map(ContainerReplicaOp::getTarget)
- .collect(Collectors.toList()));
+ ReplicationManagerUtil.ExcludedAndUsedNodes excludedAndUsedNodes =
+ ReplicationManagerUtil.getExcludedAndUsedNodes(
+ new ArrayList<>(replicas), Collections.emptySet(), pendingOps,
+ replicationManager);
+ List<DatanodeDetails> excludedNodes
+ = excludedAndUsedNodes.getExcludedNodes();
excludedNodes.addAll(replicationManager.getExcludedNodes());
Review Comment:
```
excludedNodes.addAll(replicationManager.getExcludedNodes());
```
This is done only in EC under renplication handler. Shouldn't we do the same in other handlers? If so, maybe `ReplicationManagerUtil.getExcludedAndUsedNodes` would be a good place to move the call, since it already gets reference to `replicationManager`.
--
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 pull request #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #4694:
URL: https://github.com/apache/ozone/pull/4694#issuecomment-1544219321
> Hi @sodonnel, Thanks for working on this, PR LGTM. I have one question "favoredNodes" is not used in container placement scenario or do we have any plan to use?
So far as I know favoredNodes is not used anywhere, which is a same as it makes the topology code significantly more complex. Perhaps there was a plan to use it once that never got implemented, but it was before my time on the project.
--
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 #4694: HDDS-8504. ReplicationManager: Pass used and excluded node separately for Under and Mis-Replication
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel merged PR #4694:
URL: https://github.com/apache/ozone/pull/4694
--
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