HDFS-16566 Erasure Coding: Recovery may cause excess replicas when busy DN exsits #4252
Conversation
This file contains 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. |
RuinanGu
changed the title
|
💔 -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. |
jojochuang
requested changes
May 2, 2022
hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/erasurecoding.proto
Outdated
Show resolved
Hide resolved
...oop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedWriter.java
Outdated
Show resolved
Hide resolved
...-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockECReconstructionCommand.java
Outdated
Show resolved
Hide resolved
...-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestReconstructStripedBlocks.java
Outdated
Show resolved
Hide resolved
...-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestReconstructStripedBlocks.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| StripedReconstructionInfo(ExtendedBlock blockGroup, | ||
| ErasureCodingPolicy ecPolicy, byte[] liveIndices, DatanodeInfo[] sources, | ||
| DatanodeInfo[] targets, StorageType[] targetStorageTypes, | ||
| String[] targetStorageIds) { | ||
| String[] targetStorageIds, byte[] excludeReconstructedIndices) { |
There was a problem hiding this comment.
I don't understand this.
No new code exercises this code path. Why make this change?
There was a problem hiding this comment.
When the DN receive the command of EC reconstruction, the ec task is processed in ErasureCodingWorker.processErasureCodingTasks().In this function, it calls the constructor you mentioned.
(Line 126 ,ErasureCodingWorker)
|
@tasanuma @ferhui it would be great to have your review on EC PRs. @umamaheswararao @sodonnel FYI. Please check if Ozone EC is susceptible to the same issue. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Fixed the checkstyle and compatibility of erasurecoding.proto. I don't know why the UT(TestHDFSCLI) keeps failing after I fix the issues you mentioned. All UT passed in my computer and I don't think the failed UT is related. @jojochuang @ferhui |
|
💔 -1 overall
This message was automatically generated. |
|
@jojochuang Could you please take a look again?All the issues you mentioned are fixed and the failed UT is passed in my PC. |
jojochuang
requested changes
Jun 16, 2022
hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/erasurecoding.proto
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
@jojochuang Thanks for review.Already fixed it and the UT is passed in my PC. |
jojochuang
approved these changes
Jul 15, 2022
| @@ -127,7 +127,7 @@ public void processErasureCodingTasks( | |||
| reconInfo.getExtendedBlock(), reconInfo.getErasureCodingPolicy(), | |||
| reconInfo.getLiveBlockIndices(), reconInfo.getSourceDnInfos(), | |||
| reconInfo.getTargetDnInfos(), reconInfo.getTargetStorageTypes(), | |||
| reconInfo.getTargetStorageIDs()); | |||
| reconInfo.getTargetStorageIDs(), reconInfo.getExcludeReconstructedIndices()); | |||
There was a problem hiding this comment.
Probably makes more sense to make a StripedReconstructionInfo constructor that takes BlockECReconstructionInfo as the input parameter.
Let's leave this out as a follow up.
|
Merged. Thanks for the great work @RuinanGu |
RuinanGu
changed the title
tasanuma
pushed a commit
to tasanuma/hadoop
that referenced
this pull request
Oct 21, 2022
…usy DN exsits (apache#4252) (cherry picked from commit 9376b65)
melissayou
pushed a commit
to melissayou/hadoop
that referenced
this pull request
Oct 23, 2022
…usy DN exsits (apache#4252) (apache#5059) (cherry picked from commit 9376b65) Co-authored-by: RuinanGu <57645247+RuinanGu@users.noreply.github.com> Reviewed-by: Wei-Chiu Chuang <weichiu@apache.org> Reviewed-by: Ashutosh Gupta <ashugpt@amazon.com>
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Mar 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
Jira:HDFS-16566
Simple case:
RS3-2 ,[0(busy),2,3,4] (1 missing),0 is busy.
We can get liveblockIndice=[2,3,4], additionalRepl=1.So the DN will get the LiveBitSet=[2,3,4] and targets.length=1.
According to StripedWriter.initTargetIndices(), 0 will get recovered instead of 1. So the internal blocks will become [0(busy),2,3,4,0'(excess)].Although NN will detect, delete the excess replicas and recover the missing block(1) correctly after the wrong recovery of 0', I don't think this process is expected and the recovery of 0' is obviously wrong and not necessary.
How was this patch tested?
RS6-3, [0(busy),1(busy),3,4,5,6,7,8] ,2 is missing.Test the recovery of this block group to verify there is no excess during the recovery.
For code changes:
The most important modification is in BlockManager.chooseSourceDatanodes( ). Add "ExcludeReconstructed" to save the block in busy DN. StripedWriter.initTargetIndices() will exclude the blocks which are in "ExcludeReconstructed". The changes also refer to BlockECReconstructionInfo, so the modification of erasurecoding.proto is necessary.