HDFS-17003. Erasure Coding: invalidate wrong block after reporting bad blocks from datanode #5643
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
If I understand correctly, for a replicated block, if there are two corrupt block the code in InvalidateCorruptReplicas will be called when the block has been replicated correctly. At that point there will be 3 good replicas and 2 corrupt stored in the corruptReplicas map. The code in the above method will then iterate over those two and "invalidate" them on the datanodes they are stored in. For EC, the same applies, however we are sending the blockID + index of the last reported replica to both DNs. All we store in the corruptReplica map, is the block group ID (ie the block ID with the replica index stripped out) and then the list of nodes hosting it. At this point in the code we don't know what index is on each of the nodes hosting a corrupt replica. Is this correct? Its not clear to me how the fix in this PR fixes the problem: For each node stored, we get the storages for the block, which will be the nodes hosting it. Then we getStorageOnBlock and it is sure to return non-null for each of the storages, as they all host a block in the group, right? Do we not need to somehow find the replica index for the block for each of the nodes listed, and then setup the "reported block" with the correct blockID + index for that node, passing that to invalidate? Would something like this work - NOTE - I have not tested this at all: |
Contributor
Author
|
Hi, @sodonnel . Sorry for replying late and thanks your reviewing. I totally agree with you. Current fix still has problems, I will take a look at this continuously. Thanks for your advice again. |
Contributor
Author
|
Hi, @sodonnel @Hexiaoqiao @tomscut , could you please help me review the code. thanks a lot. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Have you validated this change works correctly, as I did not test the code when I wrote it? Also, do you think you could add a test somehow to reproduce the issue and also that the fix corrects it? |
Contributor
Author
@sodonnel , Sir, i test this code in our cluster. everything looks well. Yes, it is better to add an UT to reproduce the issue. |
Contributor
zhangshuyan0
left a comment
zhangshuyan0
left a comment
There was a problem hiding this comment.
It is great to correct blockId in EC situation. But I still have some doubts about the implementation of the current invalidateCorruptReplicas method. getBlockOnStorage only ensures the correct blockId of a corrupt replica, is the generation stamp still wrong in blockToInvalidate ? In other words, if a replica corrupt due to GENSTAMP_MISMATCH, Can the check on datanode pass successfully after receiving the command(Line91 below)？
Contributor
|
@hfutatzhanghb Thanks for involving me here. It makes sense to me. +1 to "add a test somehow to reproduce the issue and check if it is fixed".
To @zhangshuyan0 , IMO this is another case (Maybe replication or EC both has the issue.), Let's file another issue to track and try to fix it. |
Contributor
Author
@Hexiaoqiao Thanks for reviewing~. I have added an UT to reproduce this case.
|
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
fix checkstyle, hadoop.hdfs.TestErasureCodingPolicies and hadoop.hdfs.TestReconstructStripedFileWithRandomECPolicy were passed in my local environment. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
To @Hexiaoqiao Thanks for you suggestion. I create a JIRA for this issue and will try to fix this later. |
Contributor
Author
|
@ayushtkn Sir, could you please also help me review this PR ？ thanks a lot. |
Hexiaoqiao
reviewed
May 19, 2023
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
Leave some comments inline. JFYI.
| final Block b = blks[0].getBlock().getLocalBlock(); | ||
| final Block b2 = blks[1].getBlock().getLocalBlock(); | ||
|
|
||
| // find the first block file |
Contributor
There was a problem hiding this comment.
Please use a capital letter at the beginning of the sentences and period at the end of it for all annotation.
Contributor
Author
There was a problem hiding this comment.
Fixed. Thanks.
| } | ||
| } | ||
|
|
||
| @Test |
Contributor
There was a problem hiding this comment.
Just suggest to add some java doc for the new unit test about what do you want to cover case.
Contributor
Author
There was a problem hiding this comment.
@Hexiaoqiao , Sir, I have added some java doc. Thanks for your suggestions.
|
💔 -1 overall
This message was automatically generated. |
Contributor
Just suggest to improve all added comments following this comments if necessary.(include some other PRs you have submitted.). Please also check the checkstyle and failed unit test if it is related to this changes. Thanks. |
…d blocks from datanode
Contributor
Author
|
@Hexiaoqiao Sir, thanks for your suggestions. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
|
Contributor
Author
|
@ayushtkn Sir, feel so sorry about involving you here too late. could you also please help review this PR ? Thanks a lot. |
Contributor
Author
|
@Hexiaoqiao he @ayushtkn @sodonnel @zhangshuyan0 Hi, sir. Could you please help me to push this issue forward ? Thanks again. |
Contributor
|
Sorry for the late response. I will review this days. cc @zhangshuyan0 would you mind to take a review if have bandwidth. |
Contributor
|
@hfutatzhanghb I think the added UT still has a problem. I'm not sure if you saw this comment from me:
|
Contributor
Author
@zhangshuyan0 Hi, shuyan. I can not see your comment in this page, so sorry about it. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
sodonnel
approved these changes
Jun 6, 2023
Contributor
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
This LGTM. Thanks for following up with the changes. Lets see how the pre-commit checks look after they return.
Contributor
|
CI just came back green as I typed my review. I am happy with this now, but I will let others have a look too. |
Contributor
Author
|
Thanks a lot, sir
…---- Replied Message ----
| From | ***@***.***> |
| Date | 06/06/2023 22:22 |
| To | ***@***.***> |
| Cc | ***@***.***>***@***.***> |
| Subject | Re: [apache/hadoop] HDFS-17003. Erasure coding: invalidate wrong block after reporting bad blocks from datanode (PR #5643) |
CI just came back green as I typed my review. I am happy with this now, but I will let others have a look too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
zhangshuyan0
approved these changes
Jun 7, 2023
...roject/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestReadStripedFileWithDecoding.java
Outdated
Show resolved
Hide resolved
zhangshuyan0
reviewed
Jun 7, 2023
| NameNodeAdapter.getDatanode(ns, cluster.getDataNodes().get(dnIndex2).getDatanodeId()); | ||
|
|
||
| for (DataNode datanode : cluster.getDataNodes()) { | ||
| if (!datanode.getDatanodeUuid().equals(dnd.getDatanodeUuid()) || |
Contributor
There was a problem hiding this comment.
Should this be && instead of || ?
Contributor
Author
There was a problem hiding this comment.
@zhangshuyan0 shuyan, I think we should use || here, because when call getBlocksToInvalidateByLimit method, it will execute blockSet.pollN(limit) which will remove the block from nodeToECBlocks and these block will be added to DatanodeDescriptor object's invalidateBlocks field.
Contributor
There was a problem hiding this comment.
Sorry I am not understand. Is this condition(line 269-270) always true?
Contributor
Author
There was a problem hiding this comment.
OMG, i am sorry, i mistook your review as line 276- line 277. my bad.
Yes, here should be &&. I will fix it.
Contributor
Author
There was a problem hiding this comment.
@zhangshuyan0 Thanks shuyan for your reviewing, have fixed.
Contributor
|
LGTM. +1 from my side. Let's wait if @zhangshuyan0 has any concerns. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
changed the title
Contributor
|
Committed to trunk. Thanks @hfutatzhanghb for your contirbutions, And @zhangshuyan0, @sodonnel for the reviews! |
Hexiaoqiao
pushed a commit
that referenced
this pull request
Jun 8, 2023
…d blocks from datanode (#5643). Contributed by hfutatzhanghb. Reviewed-by: Stephen O'Donnell <[email protected]> Reviewed-by: zhangshuyan <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]> (cherry picked from commit 0e6bd09)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The description is in HDFS-17003.