HDDS-15857. Fix checksum calculation in KeyValueHandler for variable-sized chunks

[chungen0126]

What changes were proposed in this pull request?

Problem

Currently, the getChecksums method in KeyValueHandler.java fails or returns incorrect checksums when chunks have variable sizes or are smaller than bytesPerChunk.

The original code assumes all chunks (except the last one) have a fixed size equal to bytesPerChunk. This assumption is incorrect when chunks have different lengths, leading to wrong index calculations.

Crucially, when checksum verification is enabled, the DataNode must return the actual ChunkInfo (which contains chunk lengths) to the client. Without this metadata, the client cannot perceive the boundaries of each varying chunk, making it impossible to correctly map the offsets and compute/verify the checksums.

Solution

This PR resolves the issue by tracking exact chunk boundaries on both the server and client sides:

1. Server-Side (KeyValueHandler & Protocol Updates)

• Correct Indexing in getChecksums: Rewrote the checksum slice calculation in KeyValueHandler#getChecksums. Instead of calculating chunk indexes using a fixed bytesPerChunk division, we now iterate over chunks, keep track of the cumulative offset (currentChunkOffset), and determine overlap intervals between the read range and each chunk. This ensures we correctly map and collect the target checksums.
• Send Chunk Info: Modified ContainerCommandResponseBuilders#getReadBlockResponse and KeyValueHandler#readBlockImpl to populate and return the chunkInfoList to the client when performing a block read.

2. Client-Side (StreamBlockInputStream Checksum Verification)
   Boundary-Aware Verification: In StreamBlockInputStream#onNext, if the response contains chunkInfoList and checksum verification is enabled:

• Iterate through each chunk to identify the exact overlapping region between the read block and the chunk boundaries.
• Calculate correct start and end checksum indexes using bytesPerChecksum relative to that specific chunk's start.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15857

How was this patch tested?

Added testGetChecksumsWithVaryingChunkSizes and testGetChecksumsWithSmallChunks in TestKeyValueHandler.java.
Added testSmallChunksWithLargeChecksum in TestStreamRead.java.

CI: https://github.com/chungen0126/ozone/actions/runs/29381590591

[chungen0126]

Hi @szetszwo , @sodonnel ,During my review of the current Stream Read architecture, I identified a checksum calculation issue that affects both the client and server sides.This issue occurs when a client actively triggers a flush or hsync. In such cases, the resulting chunk size does not perfectly align with bytesPerChunk, even though the checksum for this chunk has already been generated. Consequently, we can no longer simply rely on the fixed bytesPerChunk length to determine which chunk corresponds to the checksum currently being calculated.Here is a breakdown of the issues on both sides:

• Client-side Issue: Currently, the response returned by the DataNode does not contain any chunk-related information. As a result, the client cannot correctly partition the retrieved data to verify the checksums.

• Server-side Issue: The server cannot return the correct set of checksums. For example, if bytesPerChecksum is 1024, and we have chunk1 (data: a) and chunk2 (data: b). Under the current logic, the response will only return the checksum data for chunk1.

To properly verify checksums on the client side, I believe it is essential to return chunkInfo. If we go down this path, we should refine the structure of ReadBlockResponseProto, as the top-level checksumData might become redundant.

Since modifying Protobuf definitions after a major release is highly problematic due to backward compatibility/wire-protocol locking, this issue could potentially block or impact the upcoming Ozone 2.2 release. I'd love to hear your thoughts on this.

[Copilot]

Pull request overview

This PR fixes streaming-read checksum handling when block chunks are not uniform (variable-sized or smaller than bytesPerChunk) by making checksum selection and verification aware of real chunk boundaries, and by extending the read-block protocol to optionally return chunk metadata needed for correct verification.

Changes:

• Reworked server-side KeyValueHandler#getChecksums to select checksum slices based on chunk overlap rather than fixed bytesPerChunk indexing.
• Extended ReadBlockResponseProto to optionally include chunkInfoList, and wired server/client to use chunk boundaries during checksum verification.
• Added unit and integration tests covering varying/small chunk sizes with larger bytesPerChecksum.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/TestStreamRead.java	Adds integration coverage for small chunks with larger checksums during streaming read.
hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto	Adds optional chunkInfoList to ReadBlockResponseProto for boundary-aware verification.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java	Adds unit tests validating checksum slicing with varying/small chunk sizes.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java	Updates checksum slicing logic and read-block response building to support variable chunk sizes.
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/ContainerCommandResponseBuilders.java	Populates chunk info into read-block responses (needs conditional sending for perf/compat).
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestStreamBlockInputStream.java	Updates test response protos to include chunk info for checksum verification paths.
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java	Adds boundary-aware checksum verification using chunkInfoList.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[yandrey321]

good catch, should be fixed

[smengcl]

The response size concern is valid, but verifyChecksum would not work here. In prod, readBlock() always calls readBlockImpl(..., false) because that param controls datanode side verification, whereas client-side checksum verification is enabled independently. Gating chunkInfoList on this parameter would therefore omit the metadata from production responses.

We need either an explicit request capability or only the chunk metadata overlapping each response, with the client using absolute ChunkInfo.offset values.

[jojochuang]

Bugbot review

Stream checksum verify misaligns chunks — hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:638-677 · Severity: high

With checksumVerify enabled (the default), verifyChecksumForReadBlock verifies each chunk overlap without expanding to per-chunk checksum boundaries or indexing into each chunk's stored checksum list. After a prefix of non-checksum-aligned chunks, block-offset alignment can still land inside a chunk mid-checksum-unit, so verification compares freshly computed checksums against the wrong stored entries and stream reads fail with OzoneChecksumException.

---

Generated-by: Cursor Bugbot

[jojochuang]

@yandrey321

[github-actions]

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

[yandrey321]

what is the line/branch coverage for the new code? Are there test that covers Checksum = None?

[yandrey321]

optional chunkInfoList keeps the wire compatible, but throwing when it's absent is not upgrade-compatible: a new client reading from an old datanode gets a hard failure on every streaming read. Please fall back to the previous verification path (or gate on datanode version) when chunkInfoList is missing.

[chungen0126]

Good point. I will update the code to fall back to the legacy checksum verification path (Checksum.verifyChecksum(data, checksumData, 0)) when readBlock.hasChunkInfoList() is false.

[yandrey321]

Please see comments above

[smengcl]

Thanks @chungen0126 for the patch.

It would also be a good idea to rebase the PR branch (merge master into this) since it is 200+ commits behind at this point.

[chungen0126]

The read starts at block offset 2048, but chunk3 starts at 1034, so 2048 is internal offset 2048 - 1034 = 1014 inside chunk3 — not a checksum-window boundary (1014 % 1024 != 0). chk3-1 is the checksum of chunk3's first window, covering block bytes [1034, 2058), whereas the returned data begins at 2048. So checksums.get(0) does not match the first bytes of the read.

The test passes only because the chunks carry placeholder labels ("chk3-1") instead of real CRCs over real bytes, so nothing ties the checksum to the data.

This assumes the read offset lands on a chunk's internal checksum boundary, but reads are only aligned to the global bytesPerChecksum grid. Once a short chunk shifts later chunks off that grid, a partial read starting inside a chunk selects/computes checksums over the wrong byte range and throws a false mismatch

Thanks @yandrey321 for pointing this out. The core issue is that the initial blockOffset was not converted to its relative offset within the containing chunk before computing the checksum alignment.

Since each chunk maintains its own checksum grid starting from its byte 0, the read alignment only needs to be adjusted for the starting offset relative to that chunk. Once the start offset is correctly aligned, the subsequent chunk processing and iteration logic remain valid. I will update the initial offset calculation accordingly.

[szetszwo]

@chungen0126 , could you review #10415 first? It has be around for a long time. We should get it merged.

[szetszwo]

@chungen0126 , thanks for reviewing #10415 ! After merging it, this probably need to be updated. Please let me know when this is ready to be reviewed.

[chungen0126]

@szetszwo Updated! Please take a look when you are available. Thanks!

[szetszwo]

@chungen0126 , thanks for working on this!

This change is quite complicated. Let's fix the server first and separate the client change in a separated JIRA.

See the comments inlined and also https://issues.apache.org/jira/secure/attachment/13084032/10764_review.patch

[szetszwo]

This is out-of-range case is not supposed to happen. So, it should throw an exception:

thrownewIllegalStateException("blockOffset " + blockOffset + " is out of bounds: " + chunkInfos);

[szetszwo]

Even for ChecksumType.NONE, it should use getChunkRelativeOffset(..).

finallongoffsetAlignment = getChunkRelativeOffset(readBlock.getOffset(), chunkInfos) % bytesPerChecksum;
longadjustedOffset = readBlock.getOffset() - offsetAlignment;

[szetszwo]

Make it a single line and remove bytesPerChunk.

@@ -2355,7 +2355,6 @@ private long readBlockImpl(ContainerCommandRequestProto request, RandomAccessFil
return 0;
}
final List<ContainerProtos.ChunkInfo> chunkInfos = blockData.getChunks();
- final int bytesPerChunk = Math.toIntExact(chunkInfos.get(0).getLen());
final ChecksumType checksumType = chunkInfos.get(0).getChecksumData().getType();
ChecksumData checksumData = null;
int bytesPerChecksum = STREAMING_BYTES_PER_CHUNK;

finalList<ByteString> checksums = getChecksums(adjustedOffset, readLength, bytesPerChecksum, chunkInfos);
LOG.debug("Read {} at adjustedOffset {}, readLength {}, bytesPerChecksum {}",
readBlock, adjustedOffset, readLength, bytesPerChecksum);

[szetszwo]

Questions: Is it possible to have chunk length not a multiple of bytesPerChecksum?

If yes, suppose

1. bytesPerChecksum is 16, and
2. chunkList(offset, length): (0, 10), (10, 20), (30, 10)

Then, how many checksums does it need? Is it 4 (= 1 + 2 + 1)?

Comments on the code:

• We should use binary search to find the startIndex
• Assert chunkList
• We should add some tests for this method.
• The calculation can be simplified:

staticvoidassertChunkInfos(List<ChunkInfo> chunkInfos, longblockOffset, intstartIndex) {
longpreviousChunkEnd = -1;
for (inti = 0; i < chunkInfos.size(); i++) {
finalChunkInfochunk = chunkInfos.get(i);
if (previousChunkEnd >= 0) {
Preconditions.assertSame(previousChunkEnd, chunk.getOffset(), "chunkOffset");
}
Preconditions.assertTrue(chunk.getLen() > 0);
finallongchunkEnd = chunk.getOffset() + chunk.getLen();
if (i < startIndex) {
Preconditions.assertTrue(blockOffset >= chunkEnd);
} elseif (i == startIndex) {
Preconditions.assertTrue(blockOffset >= chunk.getOffset());
Preconditions.assertTrue(blockOffset < chunkEnd);
} else {
Preconditions.assertTrue(blockOffset < chunk.getOffset());
}
previousChunkEnd = chunkEnd;
}
}
publicstaticvoidverifyChecksum(ByteBufferdata, ChecksumDatachecksumData, longblockOffset, List<ChunkInfo> chunkInfoList)
throwsOzoneChecksumException {
if (checksumData.getChecksumType() == ChecksumType.NONE) {
return;
}
finalintbytesPerChecksum = checksumData.getBytesPerChecksum();
finallongreadEnd = blockOffset + data.remaining();
finalintsearchIndex = Collections.binarySearch(chunkInfoList,
ChunkInfo.newBuilder().setOffset(blockOffset).build(),
Comparator.comparing(ChunkInfo::getOffset));
finalintstartIndex = searchIndex >= 0 ? searchIndex : -(searchIndex + 1);
assertChunkInfos(chunkInfoList, blockOffset, startIndex);
for (inti = startIndex; i < chunkInfoList.size(); i++) {
finalChunkInfochunk = chunkInfoList.get(i);
if (readEnd <= chunk.getOffset()) {
return;
}
finalintdataOffset = i == startIndex ? 0 : Math.toIntExact(chunk.getOffset() - blockOffset);
finallongchunkEnd = chunk.getOffset() + chunk.getLen();
finalintdataEnd = Math.toIntExact(Math.min(chunkEnd, readEnd) - blockOffset);
finalByteBufferchunkData = data.duplicate();
chunkData.position(data.position() + dataOffset);
chunkData.limit(data.position() + dataEnd);
// TODO: check the calculation of checksumIndex below finalintchecksumIndex = Math.toIntExact(chunk.getOffset() / bytesPerChecksum);
verifyChecksum(chunkData, checksumData, checksumIndex);
}
}