HDFS-17455. Fix Client throw IndexOutOfBoundsException in DFSInputStream#fetchBlockAt #6710
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
slfan1989
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
Hi @ZanderXu @Hexiaoqiao @ayushtkn @zhangshuyan0 please help me review this PR when you are free, thanks ~ |
ZanderXu
reviewed
Apr 7, 2024
There was a problem hiding this comment.
Thanks @haiyang1987 for your report. Nice catch.
Just changing the exception type is a not a good solution. For this case, InputStream should read data from the last UC block, right? If so, can you fix this logic so that the InputStream reads the last UC block instead of throwing an exception?
|
EOF Exception is needed if the offset is bigger than file length. |
|
💔 -1 overall
This message was automatically generated. |
Thanks @ZanderXu for your comment. |
|
Update PR. Hi @ZanderXu @Hexiaoqiao @ayushtkn @zhangshuyan0 please help me review this PR when you are free, thanks ~ |
|
💔 -1 overall
This message was automatically generated. |
ZanderXu
reviewed
Apr 9, 2024
| // Here locatedBlocks has been updated, need to check offset again. | ||
| // If offset to the portion of the last block, will return the last block, | ||
| // otherwise the block containing the specified offset needs to be searched again. | ||
| if (offset >= locatedBlocks.getFileLength()) { |
There was a problem hiding this comment.
Make sense. Please make the comments clearer.
/**
* After updating the locatedBlock, the block to which the offset belongs
* should be researched like {@link DFSInputStream#getBlockAt(long)}.
*/
| Path path = new Path(file); | ||
| long fileLen = 1024 * 64; | ||
| EnumSet<CreateFlag> createFlags = EnumSet.of(CREATE); | ||
| FSDataOutputStream out = fs.create(path, FsPermission.getFileDefault(), createFlags, |
There was a problem hiding this comment.
this out should be closed in the finally logic, right?
| EnumSet<CreateFlag> createFlags = EnumSet.of(CREATE); | ||
| FSDataOutputStream out = fs.create(path, FsPermission.getFileDefault(), createFlags, | ||
| fs.getConf().getInt(IO_FILE_BUFFER_SIZE_KEY, 4096), (short) 3, | ||
| fs.getDefaultBlockSize(path), null); |
There was a problem hiding this comment.
What's the default block size? 256MB? If so, please hardcode it.
| byte[] toWrite = new byte[bufferLen]; | ||
| Random rb = new Random(0); | ||
| long bytesToWrite = fileLen; | ||
| while (bytesToWrite > 0) { |
There was a problem hiding this comment.
These logic is unnecessary if you just want to write 64KB data. 2KB is enough, right? I see you just seek to 1024.
There was a problem hiding this comment.
here will write 5KB data, ensure that the test can seek to 1024 and read 1KB of data.
| while (bytesToWrite > 0) { | ||
| rb.nextBytes(toWrite); | ||
| int bytesToWriteNext = (bufferLen < bytesToWrite) ? bufferLen : (int) bytesToWrite; | ||
| out.write(toWrite, 0, bytesToWriteNext); |
There was a problem hiding this comment.
Please add some comments to show that you just want to create a file which only contains one UC block.
| int bufLen = 1024; | ||
| byte[] buf = new byte[bufLen]; | ||
| //Seek the offset to 1024. | ||
| in.seek(1024); |
There was a problem hiding this comment.
add some comments to show that the offset should be in (0, fileSize).
|
Thanks @ZanderXu for your detailed review, i will update it later. |
|
@haiyang1987 Thanks for your works. At description I see that you mentioned, This is the root cause here, right ? I am a little confused with it why here return -2 which is binary search for collections contains only one element, IIUC, it will return -1 for this case. Please correct me if i missed something. |
The binary search will return -1 if the offset is less than the smallest offset in the list. And it will return -2 if the offset is greater than the maximum offset in the list. We can reproduce by the following steps:
And another bug is that the |
|
Got it. My bad, the first feeling is out of scope will return -1 for binary search, but actually not. |
Hexiaoqiao
reviewed
Apr 9, 2024
|
Thanks @ZanderXu @Hexiaoqiao for your detailed coment. Update pr, please help me review this PR again when you are free, thanks ~ |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Apr 10, 2024
|
Hi @haiyang1987 . There are one spotbug find and another unit test failed at latest Yetus report which is not related to this changes, would you mind to fix it later? Thanks. |
Sure, I will to fix it later. Thanks~ |
|
Committed to trunk. Thanks @haiyang1987 and @ZanderXu . |
|
Thanks @ZanderXu @Hexiaoqiao for your review and merge it. |
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
https://issues.apache.org/jira/browse/HDFS-17455
When the client read data, connect to the datanode, because at this time the datanode access token is invalid will throw InvalidBlockTokenException. At this time, when call fetchBlockAt method will throw java.lang.IndexOutOfBoundsException causing read data failed.
Root case:
The client exception:
The datanode exception: