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/08/02 18:25:12 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #3645: HDDS-7039. : EC: Handle the placement policy check in ECUnderReplicat…
swamirishi opened a new pull request, #3645:
URL: https://github.com/apache/ozone/pull/3645
…ionHandler
## What changes were proposed in this pull request?
Currently ECUnderReplicationHandler did not handle the policy placement checks. This is the Jira to add that.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7039
## How was this patch tested?
Unit 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 #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r981134021
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -153,37 +166,48 @@ public Map<DatanodeDetails, SCMCommand<?>> processAndCreateCommands(
}
}
List<Integer> missingIndexes = replicaCount.unavailableIndexes(true);
+ Map<Integer, Pair<ContainerReplica, NodeStatus>> sources =
+ filterSources(replicas, deletionInFlight);
+ List<DatanodeDetails> nodes =
+ sources.values().stream().map(Pair::getLeft)
Review Comment:
I'm confused about this piece of code, but I may be missing something obvious.
There are two filters:
```
.filter(datanodeDetails ->
datanodeDetails.getPersistedOpState() ==
HddsProtos.NodeOperationalState.IN_SERVICE)
```
And
```
.filter(DatanodeDetails::isDecomissioned)
```
Are these not mutually exclusive? Ie, a node cannot be IN_SERVICE and isDecommissioned - so will this not filter everything out?
Inside filter sources, we already sort the IN_SERVICE above other states - I am wondering why we need to filter again 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 pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3645:
URL: https://github.com/apache/ozone/pull/3645#issuecomment-1259399119
After this change, will mis-replication be detected even if there are no missing or decommission indexes? We only seem to call `validateContainerPlacement()` inside the if blocks for missing or decommission, so I don't think the handler will detect and schedule commands to fix mis-replication? I thought that this is what this Jira was to fix, but perhaps we need another Jira to do that check?
--
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] umamaheswararao commented on a diff in pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r979538738
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -234,6 +234,19 @@ public HddsProtos.NodeOperationalState getPersistedOpState() {
}
}
+ /**
+ * Checks if the OperationalState is Node is Decomissioned or Decomissioning.
+ * @return True if OperationalState is Decommissioned or Decomissioning.
+ */
+ public boolean isDecomissioned() {
+ return this.getPersistedOpState() ==
+ HddsProtos.NodeOperationalState.DECOMMISSIONED ||
+ this.getPersistedOpState() ==
+ HddsProtos.NodeOperationalState.DECOMMISSIONING;
+ }
Review Comment:
Nit: Could you please avoid this multiple empty lines?
--
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] umamaheswararao commented on pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3645:
URL: https://github.com/apache/ozone/pull/3645#issuecomment-1241551878
@swamirishi there are checkstyle warnings and update the patch?
--
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] umamaheswararao commented on a diff in pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r950456081
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -186,28 +209,30 @@ public Map<DatanodeDetails, SCMCommand<?>> processAndCreateCommands(
if (decomIndexes.size() > 0) {
final List<DatanodeDetails> selectedDatanodes =
getTargetDatanodes(excludedNodes, container, decomIndexes.size());
- excludedNodes.addAll(selectedDatanodes);
- Iterator<DatanodeDetails> iterator = selectedDatanodes.iterator();
- // In this case we need to do one to one copy.
- for (ContainerReplica replica : replicas) {
- if (decomIndexes.contains(replica.getReplicaIndex())) {
- if (!iterator.hasNext()) {
- LOG.warn("Couldn't find enough targets. Available source"
- + " nodes: {}, the target nodes: {}, excluded nodes: {} and"
- + " the decommission indexes: {}",
- replicas, selectedDatanodes, excludedNodes, decomIndexes);
- break;
+ if (validatePlacement(nodes, selectedDatanodes)) {
Review Comment:
Here nodes contains decom nodes as well? If we should we check excluding decom index 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] swamirishi commented on a diff in pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r973635899
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -186,28 +209,30 @@ public Map<DatanodeDetails, SCMCommand<?>> processAndCreateCommands(
if (decomIndexes.size() > 0) {
final List<DatanodeDetails> selectedDatanodes =
getTargetDatanodes(excludedNodes, container, decomIndexes.size());
- excludedNodes.addAll(selectedDatanodes);
- Iterator<DatanodeDetails> iterator = selectedDatanodes.iterator();
- // In this case we need to do one to one copy.
- for (ContainerReplica replica : replicas) {
- if (decomIndexes.contains(replica.getReplicaIndex())) {
- if (!iterator.hasNext()) {
- LOG.warn("Couldn't find enough targets. Available source"
- + " nodes: {}, the target nodes: {}, excluded nodes: {} and"
- + " the decommission indexes: {}",
- replicas, selectedDatanodes, excludedNodes, decomIndexes);
- break;
+ if (validatePlacement(nodes, selectedDatanodes)) {
Review Comment:
Yeah, we should exclude them.
--
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] umamaheswararao commented on a diff in pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r979538738
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -234,6 +234,19 @@ public HddsProtos.NodeOperationalState getPersistedOpState() {
}
}
+ /**
+ * Checks if the OperationalState is Node is Decomissioned or Decomissioning.
+ * @return True if OperationalState is Decommissioned or Decomissioning.
+ */
+ public boolean isDecomissioned() {
+ return this.getPersistedOpState() ==
+ HddsProtos.NodeOperationalState.DECOMMISSIONED ||
+ this.getPersistedOpState() ==
+ HddsProtos.NodeOperationalState.DECOMMISSIONING;
+ }
Review Comment:
Could you please avoid this multiple empty lines?
--
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] umamaheswararao commented on a diff in pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r950457779
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -73,6 +73,20 @@ public ECUnderReplicationHandler(
this.nodeManager = nodeManager;
}
+ private boolean validatePlacement(List<DatanodeDetails> replicaNodes,
+ List<DatanodeDetails> selectedNodes) {
+ List<DatanodeDetails> nodes = new ArrayList<>(replicaNodes);
+ nodes.addAll(selectedNodes);
+ boolean placementStatus = containerPlacement
+ .validateContainerPlacement(nodes, nodes.size())
+ .isPolicySatisfied();
+ if (!placementStatus) {
+ LOG.warn("Selected Nodes does not satisfy placement policy: {} " +
Review Comment:
Typo: Nodes - nodes and please put a period before starting next sentence.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java:
##########
@@ -73,6 +73,20 @@ public ECUnderReplicationHandler(
this.nodeManager = nodeManager;
}
+ private boolean validatePlacement(List<DatanodeDetails> replicaNodes,
+ List<DatanodeDetails> selectedNodes) {
+ List<DatanodeDetails> nodes = new ArrayList<>(replicaNodes);
+ nodes.addAll(selectedNodes);
+ boolean placementStatus = containerPlacement
+ .validateContainerPlacement(nodes, nodes.size())
+ .isPolicySatisfied();
+ if (!placementStatus) {
+ LOG.warn("Selected Nodes does not satisfy placement policy: {} " +
Review Comment:
Are you missed to pass the params 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] umamaheswararao merged pull request #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged PR #3645:
URL: https://github.com/apache/ozone/pull/3645
--
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 #3645: HDDS-7039. EC: Handle the placement policy check in ECUnderReplicationHandler
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3645:
URL: https://github.com/apache/ozone/pull/3645#discussion_r980143190
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java:
##########
@@ -234,6 +234,19 @@ public HddsProtos.NodeOperationalState getPersistedOpState() {
}
}
+ /**
+ * Checks if the OperationalState is Node is Decomissioned or Decomissioning.
+ * @return True if OperationalState is Decommissioned or Decomissioning.
+ */
+ public boolean isDecomissioned() {
+ return this.getPersistedOpState() ==
+ HddsProtos.NodeOperationalState.DECOMMISSIONED ||
+ this.getPersistedOpState() ==
+ HddsProtos.NodeOperationalState.DECOMMISSIONING;
+ }
Review Comment:
done
--
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