-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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-17033. Update fsck to display stale state info of blocks accurately #5815
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.
Most LGTM. Will +1 from my side once the following nit comments are fixed.
Configuration conf = new Configuration(); | ||
|
||
// Shorten dfs.namenode.stale.datanode.interval for easier testing. | ||
conf.set(DFS_NAMENODE_STALE_DATANODE_INTERVAL_KEY, String.valueOf(5000)); |
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.
It should be setLong
directly here.
|
||
// Make the block on datanode go into stale. | ||
cluster.stopDataNode(0); | ||
Thread.sleep(7000); |
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.
Please try to use GenericTestUtils.waitFor
instead Thread.sleep
.
|
||
File builderBaseDir = new File(GenericTestUtils.getRandomizedTempPath()); | ||
cluster = new MiniDFSCluster.Builder(conf, builderBaseDir) | ||
.numDataNodes(numDn).hosts(hosts).racks(racks).build(); |
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.
When constructing a new MiniDFSCluster, the default number of dn is 1, you can drop.numDataNodes(numDn)
.
🎊 +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
@YuanbenWang Thanks for your contribution! We need to fix checkstyle. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao @zhtttylz @slfan1989 Thank you for your suggestions! I have changed my code and could you please review it again? The failed unit test looks unrelated. |
Thanks for your review! |
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. Will commit if no other comments util two work days later.
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
Committed to trunk. Thanks @YuanbenWang for your contributions and everyone to reviews! |
@Hexiaoqiao @xinglin @zhtttylz @slfan1989 Thank your for the review! |
…ely. (apache#5815). Contributed by Wang Yuanben. Reviewed-by: Shilun Fan <[email protected]> Reviewed-by: Xing Lin <[email protected]> Reviewed-by: Hualong Zhang <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
JIRA: HDFS-17033. Update fsck to display stale state info of blocks accurately.