HDDS-9648. Create API to fetch single datanode related information #5856
Conversation
myskov
left a comment
myskov
left a comment
There was a problem hiding this comment.
Why did you decide to add an additional request instead of extending queryNode?
| } | ||
| } catch (NodeNotFoundException e) { | ||
| throw new IOException( | ||
| "An unexpected error occurred querying the NodeStatus", e); |
There was a problem hiding this comment.
"An unexpected error" is a rather particular error, not unexpected
There was a problem hiding this comment.
I have used the same message used in queryNode() method earlier. I'm not sure what you are suggesting..
There was a problem hiding this 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.
hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
Outdated
Show resolved
Hide resolved
I have overloaded the queryNode method in my current approach. Please review it and let me know your comments. Thank you. |
| for (DatanodeDetails node : scm.getScmNodeManager().getAllNodes()) { | ||
| try { | ||
| if (node.getUuid().equals(uuid)) { | ||
| NodeStatus ns = scm.getScmNodeManager().getNodeStatus(node); |
There was a problem hiding this 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.
There was a problem hiding this 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?
|
Looks largely good. I just had one comment I left inline and there is also a conflict which needs resolved on TestListInfoSubcommand.java. |
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
LGTM. We can commit when the CI goes green.
|
Thanks @Tejaskriya for the patch, @myskov, @sodonnel for the review. |
4 participants
What changes were proposed in this pull request?
Currently, various
ozone admin datanodecommands usescmClient.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