chungen0126 commented on code in PR #10764:
URL: https://github.com/apache/ozone/pull/10764#discussion_r3923159038
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2445,6 +2462,64 @@ static List<ByteString> getChecksums(long blockOffset,
int readLength, int bytes
return checksums;
}
+ /**
+ * We have to align the read to checksum boundaries, so whatever offset is
requested, we have to move back to the
+ * previous checksum boundary.
+ * eg if bytesPerChecksum is 512, and the requested offset is 600, we have
to move back to 512.
+ * Returns the checksum boundaries of {@code ChecksumBoundaries} relative to
blockOffset and blockLength.
+ */
+ private static ChecksumBoundaries getChecksumBoundaries(long blockOffset,
long blockLength,
+ List<ContainerProtos.ChunkInfo> chunkInfos, long bytesPerChecksum) {
+ final int offsetChunkIndex = searchChunkByOffset(blockOffset, chunkInfos);
+ final long offsetAlignment = (blockOffset -
chunkInfos.get(offsetChunkIndex).getOffset()) % bytesPerChecksum;
+ final long adjustedOffset = blockOffset - offsetAlignment;
+ final long blockEnd = blockOffset + blockLength - 1;
+ final ContainerProtos.ChunkInfo lastChunk =
chunkInfos.get(searchChunkByOffset(blockEnd, chunkInfos));
+
+ final long chunkOffset = lastChunk.getOffset();
+ final long chunkLength = Math.min(
+ (getEndChecksumIndex(blockEnd, chunkOffset, bytesPerChecksum) + 1) *
bytesPerChecksum, lastChunk.getLen());
+ return new ChecksumBoundaries(offsetChunkIndex, adjustedOffset,
+ chunkOffset + chunkLength - adjustedOffset);
+ }
+
+ private static int getEndChecksumIndex(long blockEnd, long chunkOffset, long
bytesPerChecksum) {
+ return (int) ((blockEnd - chunkOffset) / bytesPerChecksum);
+ }
+
+ private static final class ChecksumBoundaries {
Review Comment:
Thanks @szetszwo for the suggestion! I’d like to walk through the logic and
clarify why adjustEnd and the buffer alignment were designed this way.
Let's consider an example:
bytesPerChecksum = 16
Chunks (offset, length):
Chunk 1: (0, 10)
Chunk 2: (10, 20)
Chunk 3: (30, 10)
Chunk 4: (40, 10)
Requested read: readOffset = 16, readLength = 20 (reading bytes in range
[16, 36))
Here is the breakdown:
1. Locate start chunk: searchChunk(16) finds Chunk 2 (offset = 10).
2. Find checksum start: The checksum boundary covering offset 16 is [10, 26)
(the first 16-byte checksum block of Chunk 2).
3. Adjust start offset: Offset is moved back to 10.
4. Locate end chunk: The last requested byte is at position 35 (readOffset +
readLength - 1), which lands in Chunk 3 ([30, 40)).
5. Find checksum end: The checksum boundary covering byte 35 is Chunk 3's
checksum block [30, 40). Note that Chunk 3 has length 10 (< 16), so its padded
checksum covers the entire chunk ending at 40.
6. Adjust read length: To ensure the client receives complete checksum
boundaries to verify against, we must read from 10 up to 40, meaning
adjustedLength = 30.
7. Buffer slicing during read: When streaming the data into buffers:
1. To verify checksums incrementally, each buffer slice must align with
a complete checksum block boundary relative to chunk offsets.
2. We find the chunk containing the last byte of the candidate buffer
and align the length down to the nearest multiple of bytesPerChecksum: (len /
bytesPerChecksum) * bytesPerChecksum. Since bytesPerChecksum is a power of 2, &
-bytesPerChecksum does this directly.
Addressing the specific points:
> blockEnd is inclusive (included the last byte) so getEndChecksumIndex
should use
>
> * (blockEnd + 1 - chunkOffset) / bytesPerChecksum
>
> I suggest to make blockEnd exclusive (The computation usual is simpler.
That why most Java API use inclusive begin and exclusive end.)
Why use inclusive for blockEnd?
Sure, I can switch blockEnd to exclusive if you prefer. However, from my
perspective, this doesn't really simplify the code.
In steps 4 and 5, identifying which chunk and checksum cover the final byte
is most straightforward when using the inclusive index of that byte ((blockEnd
- chunkOffset) / bytesPerChecksum). If we use an exclusive readEnd = 36, it
correctly locates Chunk 3, but translating an exclusive boundary into the
corresponding checksum count/index requires extra handling for edge
cases—specifically when readEnd aligns exactly on a checksum boundary.
Ultimately, it just shifts the +1/-1 adjustments around rather than eliminating
them.
> Math.min should not be required -- we should always have adjusted length
<= lastChunk.getLen(). Change it to an assertion.
Why Math.min instead of plain adjustEnd?
In step 6, if a chunk's length is smaller than a multiple of
bytesPerChecksum (like Chunk 3 with length 10), adjustEnd with bytesPerChecksum
- remainder would extend the end to 30 + 16 = 46 (overshooting Chunk 3 and
bleeding 6 bytes into Chunk 4). We cannot read beyond Chunk 3's end just to pad
to 16 bytes; Chunk 3 ends at 40, so we must clamp to chunkOffset +
chunk.getLen() via Math.min.
> Sorry that I don't understand these few lines.
The bitwise operation (bufferLimit ... & -bytesPerChecksum):
This is specifically implementing step 7 (aligning buffer slices down to
checksum multiples relative to chunk offsets).
Does this align with your thoughts on how variable/small chunks should be
bounded during reads?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
