HDDS-12918. OM support byte range GET#10166
Conversation
Gargi-jais11
left a comment
Gargi-jais11
left a comment
There was a problem hiding this comment.
Thanks @ivandika3 for the patch.
Please find inlined review comments
| int partNumber, long startOffset, | ||
| long endOffset) throws IOException { |
There was a problem hiding this comment.
Nit: Please use 4 indent space
| if (!filteredLocations.isEmpty()) { | ||
| keyInfo.setByteRangeStartOffset(byteRangeStart - firstBlockStart); | ||
| } |
There was a problem hiding this comment.
I think we should also set keyInfo.setDataSize (byteRangeEnd - byteRangeStart + 1) as OzoneKeyDetails uses itRpcClient.getOzoneKeyDetails(...) does something like new OzoneKeyDetails(..., keyInfo.getDataSize(), ...).
If you don’t update dataSize after filtering, getDataSize() still looks like the whole object / whole part, while getContent() only covers some blocks — confusing and wrong for anything that uses getDataSize() for buffers, metrics etc.
| if (!filteredLocations.isEmpty()) { | |
| keyInfo.setByteRangeStartOffset(byteRangeStart - firstBlockStart); | |
| } | |
| if (!filteredLocations.isEmpty()) { | |
| keyInfo.setByteRangeStartOffset(byteRangeStart - firstBlockStart); | |
| } | |
| keyInfo.setDataSize (byteRangeEnd - byteRangeStart + 1) |
There was a problem hiding this comment.
We can also add test assert for dataSize in TestKeyManagerImpl to check if it is set correctly or not.
peterxcli
left a comment
peterxcli
left a comment
There was a problem hiding this comment.
Thanks @ivandika3 for the patch, I'm looking if we could remove the setByteRangeStartOffset, calculate the start offset at s3g side, it take the first block of block list and use the range info from header to calculate the offset, then use the result as input stream's seek offset
| * @return {@link OzoneKey} | ||
| * @throws IOException | ||
| */ | ||
| default OzoneKeyDetails getS3KeyDetails(String bucketName, String keyName, |
There was a problem hiding this comment.
why there is a implementation?
There was a problem hiding this comment.
this should sit in ClientProtocolStub.
| // Byte range requested by S3 GET. OM uses it to return only the | ||
| // key locations needed to satisfy the read. | ||
| optional uint64 byteRangeStart = 25; | ||
| optional uint64 byteRangeEnd = 26; |
There was a problem hiding this comment.
How about introducing a common msg, I remember few DN-SCM pb messages have range concept, maybe we could reuse theirs?
message ByteRange {
// Inclusive start and end offsets: [start, end].
optional uint64 start = 1;
optional uint64 end = 2;
}
message KeyArgs {
// ...
optional ByteRange byteRange = 25;
}| keyArgs.getForceUpdateContainerCacheFromSCM()) | ||
| .setMultipartUploadPartNumber(keyArgs.getMultipartNumber()) | ||
| .build(); | ||
| if (keyArgs.hasByteRangeStart() && keyArgs.hasByteRangeEnd()) { |
There was a problem hiding this comment.
could become
| if (keyArgs.hasByteRangeStart() && keyArgs.hasByteRangeEnd()) { | |
| if (keyArgs.hasByteRange()) { |
if we introduce new range message.
https://github.com/apache/ozone/pull/10166/changes#r3180692841
3 participants
What changes were proposed in this pull request?
Similar to HDDS-11699, we can move the byte range GET filter from S3G to the OM so that the network and serialization cost can be reduced for objects with a lot of blocks.
Applications like JuiceFS on S3 uses quite a bit of byte range fetch.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12918
How was this patch tested?
UT, IT.