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 2023/12/22 07:45:20 UTC
[PR] HDDS-9648. Create API to fetch single datanode related information [ozone]
Tejaskriya opened a new pull request, #5856:
URL: https://github.com/apache/ozone/pull/5856
## What changes were proposed in this pull request?
Currently, various `ozone admin datanode` commands use `scmClient.queryNode()` to retrieve datanode information. In case information about one datanode is needed, the only way to get it is to query all nodes information from the server and then filter it on the client side based on uuid or hostname, etc.
This PR introduces an API to fetch one datanode specific information through uuid of the datanode. This would reduce the data being transferred from the server to client.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9648
## How was this patch tested?
Added unit test in TestListInfoSubcommand
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5856:
URL: https://github.com/apache/ozone/pull/5856#issuecomment-1900691245
Thanks @Tejaskriya for the patch, @myskov, @sodonnel for the review.
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "myskov (via GitHub)" <gi...@apache.org>.
myskov commented on code in PR #5856:
URL: https://github.com/apache/ozone/pull/5856#discussion_r1436142877
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -612,6 +612,29 @@ public List<HddsProtos.Node> queryNode(
return result;
}
+ @Override
+ public HddsProtos.Node querySingleNode(String uuid)
+ throws IOException {
+ HddsProtos.Node result = null;
+ for (DatanodeDetails node : scm.getScmNodeManager().getAllNodes()) {
+ try {
+ if (node.getUuid().toString().equals(uuid)) {
+ NodeStatus ns = scm.getScmNodeManager().getNodeStatus(node);
+ result = HddsProtos.Node.newBuilder()
+ .setNodeID(node.getProtoBufMessage())
+ .addNodeStates(ns.getHealth())
+ .addNodeOperationalStates(ns.getOperationalState())
+ .build();
+ break;
+ }
+ } catch (NodeNotFoundException e) {
+ throw new IOException(
+ "An unexpected error occurred querying the NodeStatus", e);
Review Comment:
"An unexpected error" is a rather particular error, not unexpected
##########
hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto:
##########
@@ -326,6 +329,14 @@ message NodeQueryResponseProto {
repeated Node datanodes = 1;
}
+message SingleNodeQueryRequestProto {
+ required string uuid = 1;
Review Comment:
It's better to use UUID type instead of string
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5856:
URL: https://github.com/apache/ozone/pull/5856#discussion_r1458516070
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -612,6 +613,29 @@ public List<HddsProtos.Node> queryNode(
return result;
}
+ @Override
+ public HddsProtos.Node queryNode(UUID uuid)
+ throws IOException {
+ HddsProtos.Node result = null;
+ for (DatanodeDetails node : scm.getScmNodeManager().getAllNodes()) {
+ try {
+ if (node.getUuid().equals(uuid)) {
+ NodeStatus ns = scm.getScmNodeManager().getNodeStatus(node);
Review Comment:
Thank you for the review! I have made the changes required to incorporate your suggestion and resolved the conflicts as well. Could you please review it and approve the workflows if the patch is 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-9648. Create API to fetch single datanode related information [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5856:
URL: https://github.com/apache/ozone/pull/5856#discussion_r1455225645
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -612,6 +612,29 @@ public List<HddsProtos.Node> queryNode(
return result;
}
+ @Override
+ public HddsProtos.Node querySingleNode(String uuid)
+ throws IOException {
+ HddsProtos.Node result = null;
+ for (DatanodeDetails node : scm.getScmNodeManager().getAllNodes()) {
+ try {
+ if (node.getUuid().toString().equals(uuid)) {
+ NodeStatus ns = scm.getScmNodeManager().getNodeStatus(node);
+ result = HddsProtos.Node.newBuilder()
+ .setNodeID(node.getProtoBufMessage())
+ .addNodeStates(ns.getHealth())
+ .addNodeOperationalStates(ns.getOperationalState())
+ .build();
+ break;
+ }
+ } catch (NodeNotFoundException e) {
+ throw new IOException(
+ "An unexpected error occurred querying the NodeStatus", e);
Review Comment:
I guess "NodeNotFound" should not happen, as we have just retrieved the node from NodeManager before querying its status, so the error really is unexpected. The message includes the stack trace, so it should be easy for a caller to figure out what was wrong.
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5856:
URL: https://github.com/apache/ozone/pull/5856#discussion_r1455193349
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -612,6 +613,29 @@ public List<HddsProtos.Node> queryNode(
return result;
}
+ @Override
+ public HddsProtos.Node queryNode(UUID uuid)
+ throws IOException {
+ HddsProtos.Node result = null;
+ for (DatanodeDetails node : scm.getScmNodeManager().getAllNodes()) {
+ try {
+ if (node.getUuid().equals(uuid)) {
+ NodeStatus ns = scm.getScmNodeManager().getNodeStatus(node);
Review Comment:
Node manager has an API to get a node by UUID directly:
```
public DatanodeDetails getNodeByUuid(UUID uuid);
public DatanodeDetails getNodeByUuid(String uuid);
```
So it would be better to use that than iterate all 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
Re: [PR] HDDS-9648. Create API to fetch single datanode related information [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5856:
URL: https://github.com/apache/ozone/pull/5856
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5856:
URL: https://github.com/apache/ozone/pull/5856#issuecomment-1895555305
Looks largely good. I just had one comment I left inline and there is also a conflict which needs resolved on TestListInfoSubcommand.java.
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #5856:
URL: https://github.com/apache/ozone/pull/5856#issuecomment-1876703167
> Why did you decide to add an additional request instead of extending queryNode?
I have overloaded the queryNode method in my current approach. Please review it and let me know your comments. Thank you.
--
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-9648. Create API to fetch single datanode related information [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #5856:
URL: https://github.com/apache/ozone/pull/5856#discussion_r1440131434
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -612,6 +612,29 @@ public List<HddsProtos.Node> queryNode(
return result;
}
+ @Override
+ public HddsProtos.Node querySingleNode(String uuid)
+ throws IOException {
+ HddsProtos.Node result = null;
+ for (DatanodeDetails node : scm.getScmNodeManager().getAllNodes()) {
+ try {
+ if (node.getUuid().toString().equals(uuid)) {
+ NodeStatus ns = scm.getScmNodeManager().getNodeStatus(node);
+ result = HddsProtos.Node.newBuilder()
+ .setNodeID(node.getProtoBufMessage())
+ .addNodeStates(ns.getHealth())
+ .addNodeOperationalStates(ns.getOperationalState())
+ .build();
+ break;
+ }
+ } catch (NodeNotFoundException e) {
+ throw new IOException(
+ "An unexpected error occurred querying the NodeStatus", e);
Review Comment:
I have used the same message used in queryNode() method earlier. I'm not sure what you are suggesting..
--
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