-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-17197. Show file replication when listing corrupt files. #6095
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
Conversation
💔 -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.
Thanks @zhangshuyan0 for your contribution. Leave nit comments inline, FYI.
corruptFiles.add(new CorruptFileBlockInfo(src, blk)); | ||
int repl = 0; | ||
if (inode.isFile()) { | ||
repl = inode.asFile().getFileReplication(); |
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.
What will be return if this inode is one EC file here?
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.
Oh, I forgot to take this situation into account. How about printing -1 in CorruptFileBlockInfo
when this is a EC file?
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.
Add another attribution about ec policy and with this attribution when meet ec file, what do you think about?
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.
I think it's ok. I've updated this PR. Please take a look when you free.
} | ||
|
||
@Override | ||
public String toString() { | ||
return block.getBlockName() + "\t" + path; | ||
return block.getBlockName() + "\t" + replication + "\t" + path; |
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.
I am a little worried about the compatibility while upstream dependency with this string but we change the format here.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -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.
LGTM. +1 from my side. Let's wait if any other comments.
Committed to trunk. Thanks @zhangshuyan0 . |
Addendum: the failed unit test is not related to this PR. |
…#6095). Contributed by Shuyan Zhang. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
Description of PR
Files with different replication have different reliability guarantees. We need to pay attention to corrupted files with a specified replication greater than or equal to 3. So, when listing corrupt files, it would be useful to display the corresponding replication of the files.