HDFS-16286. Add a debug tool to verify the correctness of erasure coding on file #3593
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
cndaimin
changed the title
|
💔 -1 overall
This message was automatically generated. |
sodonnel
reviewed
Nov 1, 2021
| outputs[i].clear(); | ||
| outputs[i].limit(buffers[0].limit()); | ||
| } | ||
| this.decoder.decode(inputs, erasedIndices, outputs); |
Contributor
There was a problem hiding this comment.
I think you could make this slightly simpler by just using the encoder, rather than decoder. Simply take the data buffers and encode them to generate the parity. Then you don't need to form the missing indexes array.
Contributor
Author
There was a problem hiding this comment.
@sodonnel Thanks for your review.
Yes, encoder is better than decoder, fixed.
sodonnel
reviewed
Nov 1, 2021
| blockReaders[i] = blockReader; | ||
| } | ||
| assert checksum != null; | ||
| int bytesPerChecksum = checksum.getBytesPerChecksum(); |
Contributor
There was a problem hiding this comment.
Why do we need to adjust the read size based on the checksum size? I think the checksums are validated automatically by the underlying block reader if checksum is enabled, so we should not need to worry about that here.
Contributor
Author
There was a problem hiding this comment.
I think the adjustion based on checksum size is just for performance purpose instead of correctness. The minimum read unit on DN is the checksum size. It will avoid IO waste when the client read is well aligned by checksum.
sodonnel
reviewed
Nov 1, 2021
Contributor
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
This is a great additional to the EC tools in HDFS. I just have a couple of comments inline.
Also, do you think you could add a test or two for this new class, as it would help catch issue if someone makes changes later.
Contributor
Author
|
@sodonnel Thanks for your review. |
|
💔 -1 overall
This message was automatically generated. |
sodonnel
reviewed
Nov 2, 2021
| DFSTestUtil.createFile(fs, new Path(ecDir, "foo_5m"), 5 * m, repl, seed); | ||
| assertTrue(runCmd(new String[]{"verifyEC", "-file", "/ec/foo_5m"}) | ||
| .contains("All EC block group status: OK")); | ||
|
|
Contributor
There was a problem hiding this comment.
Could you add one more test case for a file that has multiple block groups, so we test the command looping over more than 1 block? You are using EC 3-2, so write a file that is 6MB, with a 1MB block size. That should create 2 block groups, with a length of 3MB each. Each block would then have a single 1MB EC chunk in it.
In DFSTestUtil there is a method to pass the blocksize already, so the test would be almost the same as the ones above:
public static void createFile(FileSystem fs, Path fileName, int bufferLen,
long fileLen, long blockSize, short replFactor, long seed)
Contributor
Author
There was a problem hiding this comment.
Thanks, that's a good advice, updated.
Contributor
|
Thanks for the update @cndaimin - There is just one style issue detected and I have one suggestion about adding another test case inside your existing test. Aside from that, I think this change looks good. |
Contributor
Author
|
@sodonnel Thanks for your review. |
|
🎊 +1 overall
This message was automatically generated. |
sodonnel
approved these changes
Nov 3, 2021
Contributor
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
+1 LGTM. I will commit this shortly.
Contributor
|
@cndaimin I was about to commit this, and I remembered we should update the documentation to include this command. The documentation is in a markdown file and gets published with the release, like here: That page is generated from: Would you be able to add a section for this new command under the Debug_Commands section please? |
Contributor
Author
|
@sodonnel Thanks, documentation file |
Contributor
|
Thanks, looks good. I will commit when the CI checks come back. |
|
💔 -1 overall
This message was automatically generated. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
… erasure coding on file (apache#3593) (cherry picked from commit a21895a) (cherry picked from commit 2844b98) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DebugAdmin.java Change-Id: If29f2fce5ac3c03b94a6065086a6c700aaf88c8a
symious
pushed a commit
to symious/hadoop
that referenced
this pull request
Nov 21, 2024
…ctness of erasure coding on file (apache#3593) (cherry picked from commit a21895a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This patch is tested by
hdfs debugtool.Check a good file:
Check a bad file:
Help message: