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: [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]
