-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Fix small reads of multi-part blobs #54573
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
Fix small reads of multi-part blobs #54573
Conversation
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
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
Thanks David for catching and fixing this and sorry for this bug. I left a suggestion, feel free to use it or not.
(currentPart == startPart) ? getRelativePositionInPart(position) : 0L, | ||
(currentPart == endPart) ? getRelativePositionInPart(length) : getLengthOfPart(currentPart) | ||
startInPart, | ||
endInPart - startInPart |
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.
Thanks, I was able to reproduce and came with a similar fix.
For better readability I'd suggest to isolate single part case (and be paranoid about the length to read) from the multiple parts case, something like:
endInPart - startInPart | |
if (fileInfo.numberOfParts() == 1L) { | |
return blobContainer.readBlob(fileInfo.partName(0L), position, Math.min(length, fileInfo.partBytes(0) - position)); | |
} | |
final long startPart = getPartNumberForPosition(position); | |
final long endPart = getPartNumberForPosition(position + length); | |
return new SlicedInputStream(endPart - startPart + 1L) { | |
@Override | |
protected InputStream openSlice(long slice) throws IOException { | |
final long currentPart = startPart + slice; | |
final long startInPart = (currentPart == startPart) ? getRelativePositionInPart(position) : 0L; | |
final long endInPart | |
= (currentPart == endPart) ? getRelativePositionInPart(position + length) : getLengthOfPart(currentPart); | |
return blobContainer.readBlob(fileInfo.partName(currentPart), startInPart, endInPart - startInPart | |
); | |
} | |
}; |
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.
Sure, sounds good, see 558e68a.
...plugin/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/TestUtils.java
Outdated
Show resolved
Hide resolved
…-reads-of-multi-part-blobs
…-reads-of-multi-part-blobs
No description provided.