You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Tejaskriya (via GitHub)" <gi...@apache.org> on 2024/01/05 07:07:04 UTC
[PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Tejaskriya opened a new pull request, #5929:
URL: https://github.com/apache/ozone/pull/5929
## What changes were proposed in this pull request?
In order to see which containers are blocking the progress of the decommissioning of a datanode, a list of the IDs of under-replicated and unclosed container present in the datanode are required. We already have `ozone admin datanode status decommission` command to view which datanodes are currently in decommissioning. Adding this information as a part of this command will be helpful.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10042
## How was this patch tested?
--in progress--
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461806903
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -485,9 +491,21 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
unclosed, unClosedIDs.stream().map(
Object::toString).collect(Collectors.joining(", ")));
}
+ dn.setContainersReplicatedOnNode(underReplicatedIDs, unClosedIDs);
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn) {
+ Iterator<TrackedNode> iterator = trackedNodes.iterator();
Review Comment:
We are storing trackedNodes as Set<TrackedNodes>, not a Map. I don't think we can do a simple get()
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1463008580
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
Thank you for the review! It is fixed in the latest code. Could you please review it again?
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1453105217
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
I have refactored the common code, Could you please take a look at it now?
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1455774731
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -398,6 +398,12 @@ private boolean checkPipelinesClosedOnNode(TrackedNode dn)
private boolean checkContainersReplicatedOnNode(TrackedNode dn)
throws NodeNotFoundException {
+ Map<String, List<ContainerID>> containerOnDn = getContainersReplicatedOnNode(dn, true);
+ return (containerOnDn.get("UnderReplicated").size() == 0) && (containerOnDn.get("UnClosed").size() == 0);
+ }
+
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn, boolean updateMetrics)
Review Comment:
With this change, the client is going to trigger a somewhat expensive operation on SCM. There is potential for multiple clients to issue these calls at the same time, and we don't really have a way to throttle it.
I think it would be better, if `checkContainersReplicatedOnNode` saved the counts and ID lists, perhaps as a map inside TrackedNode. Then `getContainersReplicatedOnNode` can simply retrive the value stored inside tracked node. Note that there will be a period of time where the node is scheduled for decommission, but it has not been checked yet, so we would need to decide what to return in that case.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1446124585
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
Let's rename this method to something more appropriate, like `getContainersPendingReplication`.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461799375
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -122,6 +123,15 @@ public DatanodeDetails getDatanodeDetails() {
public long getStartTime() {
return startTime;
}
+
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode() {
+ return containersReplicatedOnNode;
+ }
+
+ public void setContainersReplicatedOnNode(List<ContainerID> underReplicated, List<ContainerID> unClosed) {
+ this.containersReplicatedOnNode.put("UnderReplicated", ImmutableList.copyOf(underReplicated));
Review Comment:
I don't think we need to make a copy of the lists here. The list are local variable so nothing can change them after we exit the method, so making a copy just adds expense. It would be find to wrap them in an Immutable list however.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461817519
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -485,9 +491,21 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
unclosed, unClosedIDs.stream().map(
Object::toString).collect(Collectors.joining(", ")));
}
+ dn.setContainersReplicatedOnNode(underReplicatedIDs, unClosedIDs);
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn) {
+ Iterator<TrackedNode> iterator = trackedNodes.iterator();
Review Comment:
Ah, you are correct. Sorry!
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1455761510
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -587,6 +588,16 @@ public void deleteContainer(long containerID) throws IOException {
}
}
+ @Override
+ public Map<String, List<ContainerID>> getContainersOnDecomNode(DatanodeDetails dn) throws IOException {
+ try {
+ return scm.getScmDecommissionManager().getMonitor().getContainersReplicatedOnNode(
Review Comment:
I think it would be cleaner if we did not expose the monitor outside of ScmDecommissionManager. Instead, create a method on ScmDecommissionManager like `getContainersReplicatedOnNode(DatanodeDetails dn)` and have it return the details it gets from the monitor.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1462815013
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestDatanodeAdminMonitor.java:
##########
@@ -837,6 +837,54 @@ public void testCancelledNodesMovedToInService()
nodeManager.getNodeStatus(dn1).getOperationalState());
}
+ @Test
+ public void testContainersReplicatedOnDecomDnAPI()
+ throws NodeNotFoundException, ContainerNotFoundException {
+ conf.setBoolean("hdds.scm.replication.enable.legacy", true);
Review Comment:
Should be `false` since we use the new Replication Manager now. The default is also false.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461795801
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -485,9 +491,21 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
unclosed, unClosedIDs.stream().map(
Object::toString).collect(Collectors.joining(", ")));
}
+ dn.setContainersReplicatedOnNode(underReplicatedIDs, unClosedIDs);
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn) {
+ Iterator<TrackedNode> iterator = trackedNodes.iterator();
Review Comment:
I don't think you need to iterator here. Can you just do `trackedNodes.get(dn)`, as the `dn` is a TrackedNode.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1462801948
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
The refactor looks good, but I think you forgot to change the method's name.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1463011075
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -588,6 +589,16 @@ public void deleteContainer(long containerID) throws IOException {
}
}
+ @Override
+ public Map<String, List<ContainerID>> getContainersOnDecomNode(DatanodeDetails dn) throws IOException {
+ try {
+ return scm.getScmDecommissionManager().getContainersReplicatedOnNode(
+ new DatanodeAdminMonitorImpl.TrackedNode(dn, 0L));
Review Comment:
I have changed it now to use DatanodeDetails instead. Only inside DatanodeAdminMonitor it uses TrackedNode to find the required node
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461483057
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -587,6 +588,16 @@ public void deleteContainer(long containerID) throws IOException {
}
}
+ @Override
+ public Map<String, List<ContainerID>> getContainersOnDecomNode(DatanodeDetails dn) throws IOException {
+ try {
+ return scm.getScmDecommissionManager().getMonitor().getContainersReplicatedOnNode(
Review Comment:
Thank you for the review! I have made the change in my recent commit to not expose the monitor.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5929:
URL: https://github.com/apache/ozone/pull/5929#issuecomment-1903943201
Looks largely good - just a few minor things to fix for the comments I left inline.
--
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
Re: [PR] HDDS-10042. Show IDs of under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1505632597
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
I have addressed the above in this pr: https://github.com/apache/ozone/pull/6293
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1462780212
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -485,9 +491,21 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
unclosed, unClosedIDs.stream().map(
Object::toString).collect(Collectors.joining(", ")));
}
+ dn.setContainersReplicatedOnNode(underReplicatedIDs, unClosedIDs);
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn) {
+ Iterator<TrackedNode> iterator = trackedNodes.iterator();
Review Comment:
We are storing trackedNodes as Set<TrackedNodes>, not Map. I don't think we can do a simple get.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461800270
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -96,8 +97,8 @@ public class DatanodeAdminMonitorImpl implements DatanodeAdminMonitor {
public static final class TrackedNode {
private DatanodeDetails datanodeDetails;
-
private long startTime = 0L;
+ private Map<String, List<ContainerID>> containersReplicatedOnNode = new HashMap<>();
Review Comment:
Because this can be read when it is being updated concurrently, I thiknk it needs to be a ConcurrentHashMap.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461815445
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DecommissionStatusSubCommand.java:
##########
@@ -83,6 +85,8 @@ public void execute(ScmClient scmClient) throws IOException {
DatanodeDetails datanode = DatanodeDetails.getFromProtoBuf(
node.getNodeID());
printDetails(datanode);
+ Map<String, List<ContainerID>> containers = scmClient.getContainersOnDecomNode(datanode);
Review Comment:
Maybe should add a simple test, or extend an existing test in `TestDecommissionStatusSubCommand`. I am not sure if it will fail or not at the moment as it may not have a mock defined for the new `scmClient.getContainersOnDecomNode(datanode)` call.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1446141206
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
Also if it doesn't turn out to be too complicated, let's refactor the common code between this method and the one above to another common method. That way we don't have to maintain the same logic in two places.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1462909089
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DecommissionStatusSubCommand.java:
##########
@@ -83,6 +85,8 @@ public void execute(ScmClient scmClient) throws IOException {
DatanodeDetails datanode = DatanodeDetails.getFromProtoBuf(
node.getNodeID());
printDetails(datanode);
+ Map<String, List<ContainerID>> containers = scmClient.getContainersOnDecomNode(datanode);
Review Comment:
I have extended the tests in TestDecommissionStatusSubCommand to check for this as well now
--
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
Re: [PR] HDDS-10042. Show IDs of under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan merged PR #5929:
URL: https://github.com/apache/ozone/pull/5929
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1462908367
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -122,6 +123,15 @@ public DatanodeDetails getDatanodeDetails() {
public long getStartTime() {
return startTime;
}
+
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode() {
+ return containersReplicatedOnNode;
+ }
+
+ public void setContainersReplicatedOnNode(List<ContainerID> underReplicated, List<ContainerID> unClosed) {
+ this.containersReplicatedOnNode.put("UnderReplicated", ImmutableList.copyOf(underReplicated));
Review Comment:
Thank you for the review! I have changed this to Collections.unmodifiableList(containerList)
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1462805287
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -588,6 +589,16 @@ public void deleteContainer(long containerID) throws IOException {
}
}
+ @Override
+ public Map<String, List<ContainerID>> getContainersOnDecomNode(DatanodeDetails dn) throws IOException {
+ try {
+ return scm.getScmDecommissionManager().getContainersReplicatedOnNode(
+ new DatanodeAdminMonitorImpl.TrackedNode(dn, 0L));
Review Comment:
What do you think about changing this method's definition in `NodeDecommissionManager` to accept `DatanodeDetails` instead of `TrackedNode`? That way `SCMClientProtocolServer` or other users don't need to know about `TrackedNode` at all.
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #5929:
URL: https://github.com/apache/ozone/pull/5929#issuecomment-1905690871
@sodonnel @siddhantsangwan Thank you for the reviews! I have addressed all your comments with my latest push. Could you please review it another round and approve the workflows if everything seems good to go?
--
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
Re: [PR] HDDS-10042. Show IDs of under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1464329813
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -488,6 +488,54 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> containersReplicatedOnNode(DatanodeDetails dn)
Review Comment:
I still see the method name as `containersReplicatedOnNode` in the latest commit... am I missing something?
--
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
Re: [PR] HDDS-10042. Show IDs of under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #5929:
URL: https://github.com/apache/ozone/pull/5929#issuecomment-1907416038
Merging this now since CI is green and we have approvals. The minor name update can be taken care of in another Jira. Thanks for the code and reviews!
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461487571
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -398,6 +398,12 @@ private boolean checkPipelinesClosedOnNode(TrackedNode dn)
private boolean checkContainersReplicatedOnNode(TrackedNode dn)
throws NodeNotFoundException {
+ Map<String, List<ContainerID>> containerOnDn = getContainersReplicatedOnNode(dn, true);
+ return (containerOnDn.get("UnderReplicated").size() == 0) && (containerOnDn.get("UnClosed").size() == 0);
+ }
+
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn, boolean updateMetrics)
Review Comment:
I have followed your suggestion to save the lists of ContainerIDs in TrackedNode as a Map at the end of `checkContainersReplicatedOnNode` and `getContainersReplicatedOnNode` only retrieves this Map.
Currently, for the time period in which the node is scheduled for decommissioning but hasn't been checked yet, an empty map is shown in the place for the container IDs.
Could you please review the PR with these recent changes?
--
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
Re: [PR] HDDS-10042. Show container IDs for under-replicated and unclosed containers for decommissioning nodes [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5929:
URL: https://github.com/apache/ozone/pull/5929#discussion_r1461806903
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java:
##########
@@ -485,9 +491,21 @@ private boolean checkContainersReplicatedOnNode(TrackedNode dn)
unclosed, unClosedIDs.stream().map(
Object::toString).collect(Collectors.joining(", ")));
}
+ dn.setContainersReplicatedOnNode(underReplicatedIDs, unClosedIDs);
return underReplicated == 0 && unclosed == 0;
}
+ public Map<String, List<ContainerID>> getContainersReplicatedOnNode(TrackedNode dn) {
+ Iterator<TrackedNode> iterator = trackedNodes.iterator();
Review Comment:
We are storing trackedNodes as Set<TrackedNodes>, not a Map. I don't think we can do a simple get()
--
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