-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-16985. Fix data missing issue when delete local block file. #5564
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
…ed may lead to missing block.
zhangshuyan0
left a comment
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 Make sense to me. Failed replicas can be reported to NN when the storage back to normal state, instead of being discarded directly.
jojochuang
left a comment
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.
If there's only one replica, and AWS EBS fails, doesn't it mean the file is not recoverable anyway?
@jojochuang @zhangshuyan0 Thx for review. There may be two states:
|
|
Agree that we need to protect data for this case. But the current improvement will leave another issue. |
|
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao thx for review. Understand what you mean, removed the replica from |
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao please take a look. |
|
💔 -1 overall
This message was automatically generated. |
|
@jojochuang @Hexiaoqiao can please help take a look ? |
|
@smarthanwang Thanks for your update. it makes sense to me. Would you mind to add unit test to verify if missing block could cut down when the fail volume come back? |
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao added UT, please take a look~ |
|
@smarthanwang Please check result reported by Yetus. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao the failed UT seems not related to this PR,please help check. |
Hexiaoqiao
left a comment
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.
Leave some nit comments inline. FYI. Others seem good to me.
@jojochuang What do you think about?
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao fixed the nits,please take a look. |
Hexiaoqiao
left a comment
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 once fix the style issues.
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao fixed style issues,please take a look. |
Hexiaoqiao
left a comment
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.
Thanks for detailed review,could you help to do commit ? |
|
@jojochuang I will commit to trunk until wait two work days if no more comments. Thanks. |
|
Committed to trunk. Thanks @smarthanwang for your contribution and thanks @zhangshuyan0 @jojochuang for your reviews. |
Description of PR
see https://issues.apache.org/jira/browse/HDFS-16985
How was this patch tested?
no unit tests need
For code changes: