You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2026/09/22 17:15:52 UTC
[PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
peterxcli opened a new pull request, #11302:
URL: https://github.com/apache/ozone/pull/11302
## What changes were proposed in this pull request?
Streaming ReadBlock collects checksums assuming uniform chunks, but flushes can produce short chunks whose checksum intervals restart at different block offsets. This replaces the split range bookkeeping with `BlockReadCursor` and verifies each response against its overlapping chunks' stored checksums.
```text
Before After
readBlockImpl + ReadBlockComputation readBlockImpl + BlockReadCursor
| |
getChecksums(): assumes uniform chunks Read at chunk-relative boundaries
(can fail before sending a response) |
| Bytes + overlapping chunkInfoList
Bytes + flattened checksumData (original offsets and checksums)
| |
Client verifies one flat buffer Client verifies each chunk
|
Enqueue, then trim the seek prefix
```
With 4 bytes per checksum:
```text
Stored chunks: [ABC][DEFGHIJKL]
Stored checksum groups: [ABC][DEFG][HIJK][L]
Old flat client grouping*: [ABCD][EFGH][IJKL]
New client grouping: [ABC][DEFG][HIJK][L]
Read from offset 4 (E): receive from offset 3 (D), verify, then trim D.
* If a full response reaches the client.
```
The production path now handles variable chunks without `testVariableChunks` or the legacy flattened checksum collector. Shared verification rejects missing coverage and invalid boundaries, handles `NONE`, and preserves buffer positions. Unexpected EOF fails before sending partial checksum data; OUT_OF_RANGE retains stream error delivery and file cleanup.
**Protocol compatibility:** `ReadBlockResponseProto.checksumData` is replaced with `chunkInfoList`; field 1 and its old name are reserved. This intentionally requires matching clients and datanodes for streaming reads, which are disabled by default. There is no legacy fallback. Ordinary chunk reads and the stored format are unchanged.
## What is the link to the Apache JIRA
[HDDS-16258](https://issues.apache.org/jira/browse/HDDS-16258)
## How was this patch tested?
JDK 21: rebuilt affected modules and dependencies, including regenerated protobuf sources. All 70 focused tests passed with no skips (`TestChecksum`, `TestStreamBlockInputStream`, `TestKeyValueHandler`, `TestBlockReadCursor`, and `TestStreamRead`); checkstyle and RAT passed. The integration test inspects persisted chunks to confirm an uneven layout, then checks full and seek/range reads through `StreamBlockInputStream` with verification enabled.
Generated-by: Codex (GPT-6)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4143100426
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -585,8 +584,7 @@ public void onNext(ContainerProtos.ContainerCommandResponseProto containerComman
try {
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
- ChecksumData checksumData = ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ Checksum.validateChecksums(data, readBlock.getOffset(), 0, readBlock.getChunkInfoListList());
Review Comment:
Would a new `DatanodeVersion` for this response format let us reuse `allDataNodesSupportStreamBlock()` to select `BlockInputStream` for older datanodes at stream creation? This would avoid retaining the legacy checksum verifier. The old-client/new-datanode case would still need the agreed release note.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4087763091
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -585,8 +584,7 @@ public void onNext(ContainerProtos.ContainerCommandResponseProto containerComman
try {
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
- ChecksumData checksumData = ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ Checksum.validateChecksums(data, readBlock.getOffset(), 0, readBlock.getChunkInfoListList());
Review Comment:
Please see my earlier comment: https://github.com/apache/ozone/pull/11302#discussion_r4075277988.
Since our streaming read is off by default, I'd consider it an experimental feature. Blowing up the codebase with compatibility handling for an experimental feature makes no sense to me (and even if we did preserve compatibility, new clients would still fail in the same situation).
A few things we can do easily: 1. Add a caution to the release notes or user docs, mentioning something like: "If you want to use streaming read, don't mix datanodes running version 2.2.x with clients on 2.3+."
cc @TaiJuWu @sodonnel as you might have more context with real prod usage of streaming read.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2348,76 +2346,31 @@ private long readBlockImpl(ContainerCommandRequestProto request, RandomAccessFil
"Requested offset " + readBlock.getOffset() + " is beyond the end of block " + blockID + " with size "
+ blockData.getSize()));
}
- final List<ContainerProtos.ChunkInfo> chunkInfos = blockData.getChunks();
- final ChecksumType checksumType = chunkInfos.get(0).getChecksumData().getType();
- int bytesPerChecksum = STREAMING_BYTES_PER_CHUNK;
- if (checksumType != ContainerProtos.ChecksumType.NONE) {
- bytesPerChecksum = chunkInfos.get(0).getChecksumData().getBytesPerChecksum();
- }
-
- // TODO: Support client-side flag to toggle checksum verification.
- // If checksum is disabled, chunk offset adjustment can be skipped.
- int chunkIndex = ReadBlockComputation.searchChunk(readBlock.getOffset(), chunkInfos);
- ReadBlockComputation readBlockComputation =
- new ReadBlockComputation(responseDataSize, bytesPerChecksum, chunkInfos, chunkIndex);
- long adjustedOffset = readBlockComputation.computeAdjustedOffset(readBlock.getOffset());
-
- long adjustLength = readBlockComputation.computeAdjustedLength(
- readBlock.getOffset(), readBlock.getLength(), adjustedOffset);
-
- ChecksumData checksumData = new ChecksumData(checksumType, bytesPerChecksum);
- final ByteBuffer buffer = ByteBuffer.allocate(responseDataSize);
- blockFile.position(adjustedOffset);
- long totalDataLength = 0;
- int numResponses = 0;
- Preconditions.checkState(adjustLength <= blockData.getSize() - adjustedOffset);
- LOG.debug("adjustedOffset {}, requiredLength {}, blockSize {}",
- adjustedOffset, adjustLength, blockData.getSize());
- for (boolean shouldRead = true; totalDataLength < adjustLength && shouldRead;) {
-
- int bufferLimit = readBlockComputation.computeBufferLimit(adjustedOffset, adjustLength - totalDataLength);
-
- buffer.limit(bufferLimit);
-
- shouldRead = blockFile.read(buffer);
+ if (readBlock.getOffset() < 0 || readBlock.getLength() < 0
+ || responseDataSize < 0 || responseDataSize > OZONE_SCM_CHUNK_MAX_SIZE) {
+ return rejectReadBlock(blockFile, streamObserver, Status.INVALID_ARGUMENT.withDescription(
+ "Invalid ReadBlock range or response size: " + readBlock));
+ }
Review Comment:
I think you're right here. I'll admit that these validations were added by AI, and I'm ok to remove them because they're apparently not in the scope of either the server refactor or
1. `readBlock.getOffset() < 0 || readBlock.getLength() < 0 || responseDataSize < 0`: though our client won't send this, it can still protect the cluster from malformed grpc requests from arbitrary custom clients.
2. `responseDataSize > OZONE_SCM_CHUNK_MAX_SIZE`: this is possible with the java client, and without this check, the client will get a fatal exception. See: https://github.com/apache/ozone/blob/eec0c14b39dce6bb342ec122a74a638f79d453f7/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/ChunkUtils.java#L492-L502
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #11302:
URL: https://github.com/apache/ozone/pull/11302
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #11302:
URL: https://github.com/apache/ozone/pull/11302#issuecomment-6046419396
Thanks @peterxcli for the patch. Thanks @yandrey321 @chungen0126 @rich7420 for the reviews.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11302:
URL: https://github.com/apache/ozone/pull/11302#issuecomment-6050987008
reminder for myself: remember to add release note about the compatibility issue on jira.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4085692491
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -585,8 +584,7 @@ public void onNext(ContainerProtos.ContainerCommandResponseProto containerComman
try {
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
- ChecksumData checksumData = ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ Checksum.validateChecksums(data, readBlock.getOffset(), 0, readBlock.getChunkInfoListList());
Review Comment:
With the current approach, if a new client interacts with an older server, reading data will completely fail because chunkInfoList will be empty, which triggers an `OzoneChecksumException`. Even if checksum verification cannot be performed in that case, I think we should still ensure the client can fall back and read the data normally.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/Checksum.java:
##########
@@ -441,37 +439,39 @@ public static void verifyChecksum(List<ByteBuffer> bufferList, int startIndex, C
public static void validateChecksums(ByteBuffer data, long blockOffset, int startIndex,
Review Comment:
Adding those check is nice, but could you clarify the motivation behind refactoring this method? From my perspective, this change seems unnecessary. Please leave this part as it is.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2348,76 +2346,31 @@ private long readBlockImpl(ContainerCommandRequestProto request, RandomAccessFil
"Requested offset " + readBlock.getOffset() + " is beyond the end of block " + blockID + " with size "
+ blockData.getSize()));
}
- final List<ContainerProtos.ChunkInfo> chunkInfos = blockData.getChunks();
- final ChecksumType checksumType = chunkInfos.get(0).getChecksumData().getType();
- int bytesPerChecksum = STREAMING_BYTES_PER_CHUNK;
- if (checksumType != ContainerProtos.ChecksumType.NONE) {
- bytesPerChecksum = chunkInfos.get(0).getChecksumData().getBytesPerChecksum();
- }
-
- // TODO: Support client-side flag to toggle checksum verification.
- // If checksum is disabled, chunk offset adjustment can be skipped.
- int chunkIndex = ReadBlockComputation.searchChunk(readBlock.getOffset(), chunkInfos);
- ReadBlockComputation readBlockComputation =
- new ReadBlockComputation(responseDataSize, bytesPerChecksum, chunkInfos, chunkIndex);
- long adjustedOffset = readBlockComputation.computeAdjustedOffset(readBlock.getOffset());
-
- long adjustLength = readBlockComputation.computeAdjustedLength(
- readBlock.getOffset(), readBlock.getLength(), adjustedOffset);
-
- ChecksumData checksumData = new ChecksumData(checksumType, bytesPerChecksum);
- final ByteBuffer buffer = ByteBuffer.allocate(responseDataSize);
- blockFile.position(adjustedOffset);
- long totalDataLength = 0;
- int numResponses = 0;
- Preconditions.checkState(adjustLength <= blockData.getSize() - adjustedOffset);
- LOG.debug("adjustedOffset {}, requiredLength {}, blockSize {}",
- adjustedOffset, adjustLength, blockData.getSize());
- for (boolean shouldRead = true; totalDataLength < adjustLength && shouldRead;) {
-
- int bufferLimit = readBlockComputation.computeBufferLimit(adjustedOffset, adjustLength - totalDataLength);
-
- buffer.limit(bufferLimit);
-
- shouldRead = blockFile.read(buffer);
+ if (readBlock.getOffset() < 0 || readBlock.getLength() < 0
+ || responseDataSize < 0 || responseDataSize > OZONE_SCM_CHUNK_MAX_SIZE) {
+ return rejectReadBlock(blockFile, streamObserver, Status.INVALID_ARGUMENT.withDescription(
+ "Invalid ReadBlock range or response size: " + readBlock));
+ }
Review Comment:
Is this validation needed on the server side? The client should already be preventing these invalid requests from being sent.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4075274933
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/BlockReadCursor.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.keyvalue;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChecksumType;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChunkInfo;
+import org.apache.hadoop.ozone.container.keyvalue.helpers.ChunkUtils;
+
+/** Tracks chunk-relative checksum boundaries for a streaming block read. */
+class BlockReadCursor {
+ private static final int STREAMING_BYTES_PER_CHUNK = 1024 * 64;
Review Comment:
should it be configurable ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4075277988
##########
hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto:
##########
@@ -398,9 +398,11 @@ message ReadBlockRequestProto {
}
message ReadBlockResponseProto {
- required ChecksumData checksumData = 1;
+ reserved 1;
+ reserved "checksumData";
Review Comment:
I actually want to remove and renumber this as the streaming read is still off by default(means experimental), we could probably ignore the backward compatibility.
but that will make the check against `proto.lock` fail
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4075366627
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/BlockReadCursor.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.keyvalue;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChecksumType;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChunkInfo;
+import org.apache.hadoop.ozone.container.keyvalue.helpers.ChunkUtils;
+
+/** Tracks chunk-relative checksum boundaries for a streaming block read. */
+class BlockReadCursor {
+ private static final int STREAMING_BYTES_PER_CHUNK = 1024 * 64;
Review Comment:
That value merely preserves the old behavior for chunks written with `NONE` and is introduced in https://github.com/apache/ozone/pull/9369/changes#diff-04096cfd662f347dcf21e9e13e714af882dab2dfc02e4e9d71307b0c62b35a24R183
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4074532545
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/BlockReadCursor.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.keyvalue;
+
+import java.io.IOException;
+import java.util.List;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChecksumType;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChunkInfo;
+
+/** Tracks chunk-relative checksum boundaries for a streaming block read. */
+class BlockReadCursor {
+ private final List<ChunkInfo> chunks;
+ private final int responseDataSize;
+ private final long start;
+ private final long end;
+ private long offset;
+ private int chunkIndex;
+
+ BlockReadCursor(long requestedOffset, long length, int responseSize, List<ChunkInfo> chunks) throws IOException {
+ this.chunks = chunks;
+ long blockEnd = 0;
+ int bufferSize = responseSize;
+ for (ChunkInfo chunk : chunks) {
+ if (chunk.getOffset() != blockEnd || chunk.getLen() <= 0 || chunk.getLen() > Long.MAX_VALUE - blockEnd) {
+ throw new IOException("Invalid chunk range: " + chunk);
+ }
+ blockEnd += chunk.getLen();
+ // One checksum interval (or the short chunk containing it) must always fit in the buffer.
+ bufferSize = Math.max(bufferSize, (int) Math.min(chunk.getLen(), interval(chunk)));
+ }
Review Comment:
Buffer sizing can grow to the maximum checksum interval seen in chunk metadata, which may cause large per-request allocations (and potential OOM or GC pressure) if `bytesPerChecksum` is large or metadata is corrupted. Consider enforcing a reasonable upper bound for the computed `bufferSize` (or rejecting chunks with excessively large `bytesPerChecksum`) to protect datanode memory under unexpected inputs.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2348,76 +2345,30 @@ private long readBlockImpl(ContainerCommandRequestProto request, RandomAccessFil
"Requested offset " + readBlock.getOffset() + " is beyond the end of block " + blockID + " with size "
+ blockData.getSize()));
}
- final List<ContainerProtos.ChunkInfo> chunkInfos = blockData.getChunks();
- final ChecksumType checksumType = chunkInfos.get(0).getChecksumData().getType();
- int bytesPerChecksum = STREAMING_BYTES_PER_CHUNK;
- if (checksumType != ContainerProtos.ChecksumType.NONE) {
- bytesPerChecksum = chunkInfos.get(0).getChecksumData().getBytesPerChecksum();
+ if (readBlock.getOffset() < 0 || readBlock.getLength() < 0 || responseDataSize < 0) {
+ return rejectReadBlock(blockFile, streamObserver, Status.INVALID_ARGUMENT.withDescription(
+ "Invalid ReadBlock range or response size: " + readBlock));
}
-
- // TODO: Support client-side flag to toggle checksum verification.
- // If checksum is disabled, chunk offset adjustment can be skipped.
- int chunkIndex = ReadBlockComputation.searchChunk(readBlock.getOffset(), chunkInfos);
- ReadBlockComputation readBlockComputation =
- new ReadBlockComputation(responseDataSize, bytesPerChecksum, chunkInfos, chunkIndex);
- long adjustedOffset = readBlockComputation.computeAdjustedOffset(readBlock.getOffset());
-
- long adjustLength = readBlockComputation.computeAdjustedLength(
- readBlock.getOffset(), readBlock.getLength(), adjustedOffset);
-
- ChecksumData checksumData = new ChecksumData(checksumType, bytesPerChecksum);
- final ByteBuffer buffer = ByteBuffer.allocate(responseDataSize);
- blockFile.position(adjustedOffset);
- long totalDataLength = 0;
- int numResponses = 0;
- Preconditions.checkState(adjustLength <= blockData.getSize() - adjustedOffset);
- LOG.debug("adjustedOffset {}, requiredLength {}, blockSize {}",
- adjustedOffset, adjustLength, blockData.getSize());
- for (boolean shouldRead = true; totalDataLength < adjustLength && shouldRead;) {
-
- int bufferLimit = readBlockComputation.computeBufferLimit(adjustedOffset, adjustLength - totalDataLength);
-
- buffer.limit(bufferLimit);
-
- shouldRead = blockFile.read(buffer);
+ final BlockReadCursor cursor = new BlockReadCursor(readBlock.getOffset(), readBlock.getLength(),
+ responseDataSize, blockData.getChunks());
+ final ByteBuffer buffer = ByteBuffer.allocate(cursor.responseDataSize());
+ blockFile.position(cursor.offset());
+ while (cursor.hasRemaining()) {
+ buffer.clear().limit(cursor.nextReadLength());
+ blockFile.read(buffer);
+ if (buffer.hasRemaining()) {
+ throw new EOFException("Unexpected end of block " + blockID + " at " + cursor.offset());
+ }
Review Comment:
This treats any short read as EOF and fails the request, but file/channel reads are not guaranteed to fill the buffer in a single `read` call (short reads can happen without EOF). To avoid false EOFs, read in a loop until the buffer is filled or an actual EOF is detected (e.g., `read` returns < 0 / false with 0 bytes read).
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2348,76 +2345,30 @@ private long readBlockImpl(ContainerCommandRequestProto request, RandomAccessFil
"Requested offset " + readBlock.getOffset() + " is beyond the end of block " + blockID + " with size "
+ blockData.getSize()));
}
- final List<ContainerProtos.ChunkInfo> chunkInfos = blockData.getChunks();
- final ChecksumType checksumType = chunkInfos.get(0).getChecksumData().getType();
- int bytesPerChecksum = STREAMING_BYTES_PER_CHUNK;
- if (checksumType != ContainerProtos.ChecksumType.NONE) {
- bytesPerChecksum = chunkInfos.get(0).getChecksumData().getBytesPerChecksum();
+ if (readBlock.getOffset() < 0 || readBlock.getLength() < 0 || responseDataSize < 0) {
+ return rejectReadBlock(blockFile, streamObserver, Status.INVALID_ARGUMENT.withDescription(
+ "Invalid ReadBlock range or response size: " + readBlock));
}
-
- // TODO: Support client-side flag to toggle checksum verification.
- // If checksum is disabled, chunk offset adjustment can be skipped.
- int chunkIndex = ReadBlockComputation.searchChunk(readBlock.getOffset(), chunkInfos);
- ReadBlockComputation readBlockComputation =
- new ReadBlockComputation(responseDataSize, bytesPerChecksum, chunkInfos, chunkIndex);
- long adjustedOffset = readBlockComputation.computeAdjustedOffset(readBlock.getOffset());
-
- long adjustLength = readBlockComputation.computeAdjustedLength(
- readBlock.getOffset(), readBlock.getLength(), adjustedOffset);
-
- ChecksumData checksumData = new ChecksumData(checksumType, bytesPerChecksum);
- final ByteBuffer buffer = ByteBuffer.allocate(responseDataSize);
- blockFile.position(adjustedOffset);
- long totalDataLength = 0;
- int numResponses = 0;
- Preconditions.checkState(adjustLength <= blockData.getSize() - adjustedOffset);
- LOG.debug("adjustedOffset {}, requiredLength {}, blockSize {}",
- adjustedOffset, adjustLength, blockData.getSize());
- for (boolean shouldRead = true; totalDataLength < adjustLength && shouldRead;) {
-
- int bufferLimit = readBlockComputation.computeBufferLimit(adjustedOffset, adjustLength - totalDataLength);
-
- buffer.limit(bufferLimit);
-
- shouldRead = blockFile.read(buffer);
+ final BlockReadCursor cursor = new BlockReadCursor(readBlock.getOffset(), readBlock.getLength(),
+ responseDataSize, blockData.getChunks());
Review Comment:
A `responseDataSize` of 0 isn't rejected here, but `BlockReadCursor` rejects `responseSize <= 0` by throwing an `IOException`, which is likely mapped to an IO failure rather than an INVALID_ARGUMENT. Consider tightening the check to `responseDataSize <= 0` so client input errors are consistently rejected as INVALID_ARGUMENT.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4103765133
##########
hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto:
##########
@@ -398,9 +398,11 @@ message ReadBlockRequestProto {
}
message ReadBlockResponseProto {
- required ChecksumData checksumData = 1;
+ reserved 1;
+ reserved "checksumData";
Review Comment:
> could we document both failure scenarios and recovery steps in the release notes?
yes, that's what I want.
##########
hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto:
##########
@@ -398,9 +398,11 @@ message ReadBlockRequestProto {
}
message ReadBlockResponseProto {
- required ChecksumData checksumData = 1;
+ reserved 1;
+ reserved "checksumData";
Review Comment:
> could we document both failure scenarios and recovery steps in the release notes?
yes, that's what I want.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4087763091
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -585,8 +584,7 @@ public void onNext(ContainerProtos.ContainerCommandResponseProto containerComman
try {
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
- ChecksumData checksumData = ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ Checksum.validateChecksums(data, readBlock.getOffset(), 0, readBlock.getChunkInfoListList());
Review Comment:
Please see my earlier comment: https://github.com/apache/ozone/pull/11302#discussion_r4075277988.
Since our streaming read is off by default, I'd consider it an experimental feature. Blowing up the codebase with compatibility handling for an experimental feature makes no sense to me (and even if we did preserve compatibility, new clients would still fail in the same situation).
A few things we can do easily: 1. Add a caution to the release notes or user docs, mentioning something like: "If you want to use streaming read, don't mix datanodes running version 2.2.x with clients on 2.3+."
cc @taklwu @sodonnel as you might have more context with real prod usage of streaming read.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11302:
URL: https://github.com/apache/ozone/pull/11302#issuecomment-5987310660
@ss77892 would you like to take a look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11302:
URL: https://github.com/apache/ozone/pull/11302#issuecomment-6049469079
Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4100557173
##########
hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto:
##########
@@ -398,9 +398,11 @@ message ReadBlockRequestProto {
}
message ReadBlockResponseProto {
- required ChecksumData checksumData = 1;
+ reserved 1;
+ reserved "checksumData";
Review Comment:
With this change, an older client with streaming reads enabled would fail to parse responses after the datanodes are upgraded, even following a non-rolling cluster upgrade. The reverse, an upgraded client reading from older datanodes also fails when checksum verification is enabled because chunkInfoList is absent.
At least this is just a wire proto change and does not affect stored data. Users can disable streaming reads and reopen the client, or by bringing clients and datanodes onto compatible versions. Given that streaming reads are opt-in and this break is intentional for performance reason IIUC, I’m fine with the change for now, but could we document both failure scenarios and recovery steps in the release notes?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4103765133
##########
hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto:
##########
@@ -398,9 +398,11 @@ message ReadBlockRequestProto {
}
message ReadBlockResponseProto {
- required ChecksumData checksumData = 1;
+ reserved 1;
+ reserved "checksumData";
Review Comment:
yes, that's what I want.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Verify streaming block reads using chunk boundaries [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4087763091
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -585,8 +584,7 @@ public void onNext(ContainerProtos.ContainerCommandResponseProto containerComman
try {
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
- ChecksumData checksumData = ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ Checksum.validateChecksums(data, readBlock.getOffset(), 0, readBlock.getChunkInfoListList());
Review Comment:
Please see my earlier comment: https://github.com/apache/ozone/pull/11302#discussion_r4075277988.
Since our streaming read is off by default, I'd consider it an experimental feature. Blowing up the codebase with compatibility handling for an experimental feature makes no sense to me (and even if we did preserve compatibility, new clients would still fail in the same situation).
A few things we can do easily: 1. Add a caution to the release notes or user docs, mentioning something like: "If you want to use streaming read, don't mix datanodes running version 2.2.x with clients on 2.3+."
cc @taklwu @sodonnel @amaliujia as you might have more context with real prod usage of streaming read.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16258. Refactor streaming block reads and fix checksum verification for variable-sized chunks [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11302:
URL: https://github.com/apache/ozone/pull/11302#issuecomment-5997506592
@szetszwo would you like to take a look? thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org