-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDFS-16581. Print node status when executing printTopology #4321
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There are some tests that fail in CI/CD. E.g: It looks like these failures have little to do with the code I submitted. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me.
I am a little worried that this will change the output structure of printTopology
. I wonder if this will be an incompatible change. Do I need to add a releaseNote? @tasanuma @virajjasani Could you please give some suggestions? Thanks.
Thanks @tomscut . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few minor nits, overall change looks good. Thanks @jianghuazhu
HashMap<String, HashMap<String, String>> tree = new HashMap<String, | ||
HashMap<String, String>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you please replace this with?
Map<String, HashMap<String, String>> map = new HashMap<>();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is my mistake, I will update later.
|
||
tree.get(location).add(name); | ||
if(!tree.containsKey(location)) { | ||
tree.put(location, new HashMap<String, String>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, new HashMap<>()
is sufficient :)
ArrayList<String> racks = new ArrayList<String>(tree.keySet()); | ||
Collections.sort(racks); | ||
// Sort the racks (and nodes) alphabetically, display in order | ||
ArrayList<String> racks = new ArrayList<String>(tree.keySet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: List<String> racks = new ArrayList<>(tree.keySet())
@tomscut sounds good, we can keep this to trunk and add release note as well reg the output format change, thanks! |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Here are some failed unit tests. include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 (non-binding), thanks @jianghuazhu
Thanks @virajjasani . |
Reviewed-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Tao Li <tomscut@apache.org>
Thanks @jianghuazhu for your contribution. Thanks @virajjasani for your review. |
Reviewed-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Tao Li <tomscut@apache.org>
Description of PR
Right now we can't directly see the status of some DataNodes, it would be helpful to see this information through the dfsadmin tool.
Details: HDFS-16581
How was this patch tested?
Need to test, when nodes are in DECOMMISSION_INPROGRESS, DECOMMISSION_INPROGRESS or alive state, we should correctly identify them.