Copilot commented on code in PR #10764:
URL: https://github.com/apache/ozone/pull/10764#discussion_r3584640445
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -532,10 +534,14 @@ public void
onNext(ContainerProtos.ContainerCommandResponseProto containerComman
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
ChecksumData checksumData =
ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ if (readBlock.hasChunkInfoList()) {
+ verifyChecksumForReadBlock(data, checksumData, readBlock);
+ } else {
+ throw new IOException("Checksum data is missing for block " +
getBlockID());
+ }
Review Comment:
When checksum verification is enabled, the code currently throws if
chunkInfoList is missing, even if the checksum type is NONE (where verification
is a no-op). Also, the exception message says "Checksum data is missing" but
the missing field is chunkInfoList.
This can cause reads to fail unnecessarily (eg. checksum type NONE or older
servers that don't send chunkInfoList). Consider only requiring chunkInfoList
when checksum type != NONE, and update the message accordingly.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -628,5 +634,46 @@ public void setStreamingReadResponse(StreamingReadResponse
streamingReadResponse
public String toString() {
return name;
}
+
+ private void verifyChecksumForReadBlock(
+ ByteBuffer data, ChecksumData checksumData, ReadBlockResponseProto
readBlock)
+ throws OzoneChecksumException {
+ if (!checksumData.getChecksumType().equals(ChecksumType.NONE)) {
+ int bytesPerChecksum = checksumData.getBytesPerChecksum();
+ long blockOffset = readBlock.getOffset();
+ long readLength = data.remaining();
+ long currentChunkOffset = 0;
+ int checksumIndex = 0;
+ int dataOffset = 0;
+
+ for (ContainerProtos.ChunkInfo chunk :
readBlock.getChunkInfoList().getChunksList()) {
+ long chunkStart = currentChunkOffset;
+ long chunkEnd = chunkStart + chunk.getLen();
+
Review Comment:
verifyChecksumForReadBlock derives chunk boundaries from a cumulative
counter (currentChunkOffset) rather than using the ChunkInfo.offset values
provided by the server. If chunk offsets are not strictly contiguous / ordered,
this can compute the wrong overlap ranges and advance checksumIndex incorrectly.
Use chunk.getOffset() for chunkStart (and derive chunkEnd from it) instead
of assuming contiguous offsets.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/TestStreamRead.java:
##########
@@ -301,4 +301,44 @@ static String runTestReadKey(String name, SizeInBytes
keySize, SizeInBytes buffe
print(name, keySizeByte, elapsedNanos, bufferSize, computedMD5);
return computedMD5;
}
+
+ @Test
+ void testSmallChunksWithLargeChecksum() throws Exception {
+ final SizeInBytes bytesPerChecksum = SizeInBytes.valueOf(512);
+ System.out.println("cluster starting ...");
+ try (MiniOzoneCluster cluster = newCluster(bytesPerChecksum.getSizeInt()))
{
+ cluster.waitForClusterToBeReady();
+ System.out.println("cluster ready");
+
+ OzoneConfiguration conf = cluster.getConf();
+ OzoneClientConfig clientConfig = conf.getObject(OzoneClientConfig.class);
+ clientConfig.setStreamReadBlock(true);
+ clientConfig.setStreamBufferFlushDelay(false);
+ final OzoneConfiguration steamReadConf = new OzoneConfiguration(conf);
+ steamReadConf.setFromObject(clientConfig);
+
+ try (OzoneClient streamReadClient =
OzoneClientFactory.getRpcClient(steamReadConf)) {
Review Comment:
Typo in variable name: `steamReadConf` should be `streamReadConf` for
clarity (and to match the surrounding `streamReadClient` naming).
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/ContainerCommandResponseBuilders.java:
##########
@@ -336,14 +338,17 @@ public static ContainerCommandResponseProto
getReadChunkResponse(
}
public static ContainerCommandResponseProto getReadBlockResponse(
- ContainerCommandRequestProto request, ChecksumData checksumData,
ByteBuffer data, long offset) {
+ ContainerCommandRequestProto request, ChecksumData checksumData,
+ ByteBuffer data, long offset, List<ChunkInfo> chunkInfoList, boolean
verifyChecksum) {
- ContainerProtos.ReadBlockResponseProto response =
ContainerProtos.ReadBlockResponseProto.newBuilder()
+ ContainerProtos.ReadBlockResponseProto response =
ReadBlockResponseProto.newBuilder()
.setChecksumData(checksumData.getProtoBufMessage())
.setData(ByteString.copyFrom(data))
.setOffset(offset)
+
.setChunkInfoList(ChunkInfoList.newBuilder().addAllChunks(chunkInfoList).build())
.build();
Review Comment:
getReadBlockResponse currently always serializes and sends the full
chunkInfoList, and the verifyChecksum parameter is unused. Sending chunk
metadata on every ReadBlock response can significantly increase response sizes
(especially since readBlockImpl can stream many responses per block).
Use verifyChecksum to only attach chunkInfoList when checksum verification
is enabled (and avoid setting it at all when not needed).
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2225,30 +2225,41 @@ private long readBlockImpl(ContainerCommandRequestProto
request, RandomAccessFil
static List<ByteString> getChecksums(long blockOffset, int readLength, int
bytesPerChunk, int bytesPerChecksum,
final List<ContainerProtos.ChunkInfo> chunks) {
assertSame(0, blockOffset % bytesPerChecksum, "blockOffset %
bytesPerChecksum");
- final int numChecksums = 1 + (readLength - 1) / bytesPerChecksum;
- final List<ByteString> checksums = new ArrayList<>(numChecksums);
- for (int i = 0; i < numChecksums; i++) {
- // As the checksums are stored "chunk by chunk", we need to figure out
which chunk we start reading from,
- // and its offset to pull out the correct checksum bytes for each read.
- final int n = i * bytesPerChecksum;
- final long offset = blockOffset + n;
- final int c = Math.toIntExact(offset / bytesPerChunk);
- final int chunkOffset = Math.toIntExact(offset % bytesPerChunk);
- final int csi = chunkOffset / bytesPerChecksum;
-
- assertTrue(c < chunks.size(),
- () -> "chunkIndex = " + c + " >= chunk.size()" + chunks.size());
- final ContainerProtos.ChunkInfo chunk = chunks.get(c);
- if (c < chunks.size() - 1) {
- assertSame(bytesPerChunk, chunk.getLen(), "bytesPerChunk");
- }
- final ContainerProtos.ChecksumData checksumDataProto =
chunks.get(c).getChecksumData();
- assertSame(bytesPerChecksum, checksumDataProto.getBytesPerChecksum(),
"bytesPerChecksum");
- final List<ByteString> checksumsList =
checksumDataProto.getChecksumsList();
- assertTrue(csi < checksumsList.size(),
- () -> "checksumIndex = " + csi + " >= checksumsList.size()" +
checksumsList.size());
- checksums.add(checksumsList.get(csi));
+ final List<ByteString> checksums = new ArrayList<>();
+
+ long currentChunkOffset = 0;
+ for (ContainerProtos.ChunkInfo chunk : chunks) {
+ long chunkStart = currentChunkOffset;
+ long chunkEnd = chunkStart + chunk.getLen();
+
+ long overlapStart = Math.max(blockOffset, chunkStart);
+ long overlapEnd = Math.min(blockOffset + readLength, chunkEnd);
+
Review Comment:
getChecksums computes chunkStart/chunkEnd using a cumulative counter
(currentChunkOffset) instead of the ChunkInfo.offset values. Since ChunkInfo
already carries the exact offset, relying on a running total can return
incorrect overlaps if the chunk list is ever non-contiguous or not strictly
ordered.
Prefer using chunk.getOffset() to determine chunkStart.
--
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]
