-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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-17001. Support getStatus API in WebHDFS #5628
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@ayushtkn @slfan1989 Would you be so kind as to review my pull request, please? Thank you very much! |
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.
dropped some comments, rest lgtm
@@ -2178,6 +2179,19 @@ HdfsFileStatus decodeResponse(Map<?, ?> json) { | |||
return status.makeQualified(getUri(), f); | |||
} | |||
|
|||
@Override | |||
public FsStatus getStatus(Path f) throws IOException { |
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:
change f
to path
private long getStateAtIndex(long[] states, int index) { | ||
return states.length > index ? states[index] : -1; | ||
} |
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 same is defined in DfsClient, can we make the definition over there public static
and use it here as well, rather than defining it twice?
} | ||
|
||
public static Map<String, Object> toJsonMap(FsStatus status) | ||
throws IOException { |
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.
this doesn't throw IOE, can be removed, once you remove it from here, I think the throws IOE can be removed from the above method as well
"FsStatus": { | ||
"used": 0, | ||
"remaining": 0, | ||
"capacity":0 | ||
} |
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.
can you try it in an actual cluster, get a better example rather than having all 0
final Configuration conf = WebHdfsTestUtil.createConf(); | ||
try { | ||
cluster = new MiniDFSCluster.Builder(conf) | ||
.numDataNodes(1) |
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.
datanodes are 1 by default, this line isn't required
Assert.assertTrue(fsStatus.getUsed() >= 0); | ||
Assert.assertTrue(fsStatus.getRemaining() >= 0); | ||
Assert.assertTrue(fsStatus.getCapacity() >= 0); |
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.
there is already a static import for these. No need of Assert. prefix.
Rather than just asserting they aren't 0, can you get the values from DistributedFileSystem and validate that they are same
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.
Thank you for your valuable suggestion. I greatly appreciate it and will promptly make the necessary changes to the code!
This comment was marked as outdated.
This comment was marked as outdated.
💔 -1 overall
This message was automatically generated. |
@ayushtkn @slfan1989 Could you please help review this PR again? The |
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.
LGTM
If this isn't there in HTTPFs do have a follow up ticket to add it there as well, we try to keep things in sync with both WebHDFS & HTTPFs
dfsFsStatus.getCapacity()); | ||
} finally { | ||
cluster.shutdown(); | ||
} |
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.
@zhtttylz Thanks for the contribution! should we close the os?
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.
Thank you for your valuable suggestion. I sincerely appreciate it and will promptly implement the required adjustments to the code!
@ayushtkn Thank you very much for your valuable suggestion. We will create a ticket to add this feature to HTTPFs! |
💔 -1 overall
This message was automatically generated. |
@slfan1989 Would you kindly assist in reviewing this PR once again?The |
@ayushtkn @slfan1989 Thank you for your assistance in reviewing the code! |
JIRA: HDFS-17001. Support getStatus API in WebHDFS