szetszwo commented on code in PR #10764:
URL: https://github.com/apache/ozone/pull/10764#discussion_r3832940328
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/Checksum.java:
##########
@@ -430,4 +430,44 @@ public static void verifyChecksum(List<ByteBuffer>
bufferList, int startIndex, C
public static ContainerProtos.ChecksumData getNoChecksumDataProto() {
return new ChecksumData(ChecksumType.NONE, 0).getProtoBufMessage();
}
+
+ public static void verifyChecksum(
+ ByteBuffer data, ChecksumData checksumData, long blockOffset,
List<ContainerProtos.ChunkInfo> chunkInfoList)
+ throws OzoneChecksumException {
+ if (!checksumData.getChecksumType().equals(ChecksumType.NONE)) {
+ int bytesPerChecksum = checksumData.getBytesPerChecksum();
+ long readLength = data.remaining();
+ long currentChunkOffset = 0;
+ int checksumIndex = 0;
+ int dataOffset = 0;
+
+ for (ContainerProtos.ChunkInfo chunk : chunkInfoList) {
+ long chunkStart = currentChunkOffset;
+ long chunkEnd = chunkStart + chunk.getLen();
+
+ long overlapStart = Math.max(blockOffset, chunkStart);
+ long overlapEnd = Math.min(blockOffset + readLength, chunkEnd);
+
+ if (overlapStart < overlapEnd) {
+ int overlapLen = Math.toIntExact(overlapEnd - overlapStart);
+ ByteBuffer chunkData = data.duplicate();
+ chunkData.position(data.position() + dataOffset);
+ chunkData.limit(data.position() + dataOffset + overlapLen);
+
+ Checksum.verifyChecksum(chunkData, checksumData, checksumIndex);
+
+ dataOffset += overlapLen;
+
+ long offsetInChunk = overlapStart - chunkStart;
+ long endOffsetInChunk = overlapEnd - chunkStart;
+
+ int firstChecksumIndex = Math.toIntExact(offsetInChunk /
bytesPerChecksum);
+ int lastChecksumIndex = Math.toIntExact((endOffsetInChunk - 1) /
bytesPerChecksum);
+
+ checksumIndex += (lastChecksumIndex - firstChecksumIndex + 1);
+ }
+ currentChunkOffset += chunk.getLen();
+ }
+ }
+ }
Review Comment:
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:
```java
static void assertChunkInfos(List<ChunkInfo> chunkInfos, long blockOffset,
int startIndex) {
long previousChunkEnd = -1;
for (int i = 0; i < chunkInfos.size(); i++) {
final ChunkInfo chunk = chunkInfos.get(i);
if (previousChunkEnd >= 0) {
Preconditions.assertSame(previousChunkEnd, chunk.getOffset(),
"chunkOffset");
}
Preconditions.assertTrue(chunk.getLen() > 0);
final long chunkEnd = chunk.getOffset() + chunk.getLen();
if (i < startIndex) {
Preconditions.assertTrue(blockOffset >= chunkEnd);
} else if (i == startIndex) {
Preconditions.assertTrue(blockOffset >= chunk.getOffset());
Preconditions.assertTrue(blockOffset < chunkEnd);
} else {
Preconditions.assertTrue(blockOffset < chunk.getOffset());
}
previousChunkEnd = chunkEnd;
}
}
public static void verifyChecksum(ByteBuffer data, ChecksumData
checksumData, long blockOffset, List<ChunkInfo> chunkInfoList)
throws OzoneChecksumException {
if (checksumData.getChecksumType() == ChecksumType.NONE) {
return;
}
final int bytesPerChecksum = checksumData.getBytesPerChecksum();
final long readEnd = blockOffset + data.remaining();
final int searchIndex = Collections.binarySearch(chunkInfoList,
ChunkInfo.newBuilder().setOffset(blockOffset).build(),
Comparator.comparing(ChunkInfo::getOffset));
final int startIndex = searchIndex >= 0 ? searchIndex : -(searchIndex +
1);
assertChunkInfos(chunkInfoList, blockOffset, startIndex);
for (int i = startIndex; i < chunkInfoList.size(); i++) {
final ChunkInfo chunk = chunkInfoList.get(i);
if (readEnd <= chunk.getOffset()) {
return;
}
final int dataOffset = i == startIndex ? 0 :
Math.toIntExact(chunk.getOffset() - blockOffset);
final long chunkEnd = chunk.getOffset() + chunk.getLen();
final int dataEnd = Math.toIntExact(Math.min(chunkEnd, readEnd) -
blockOffset);
final ByteBuffer chunkData = data.duplicate();
chunkData.position(data.position() + dataOffset);
chunkData.limit(data.position() + dataEnd);
// TODO: check the calculation of checksumIndex below
final int checksumIndex = Math.toIntExact(chunk.getOffset() /
bytesPerChecksum);
verifyChecksum(chunkData, checksumData, checksumIndex);
}
}
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2404,33 +2406,63 @@ private long readBlockImpl(ContainerCommandRequestProto
request, RandomAccessFil
return totalDataLength;
}
- static List<ByteString> getChecksums(long blockOffset, int readLength, int
bytesPerChunk, int bytesPerChecksum,
+ /**
+ * If Checksum type is not NONE then we have to align the read to checksum
boundaries.
+ * Each chunk has its own checksum grid starting at byte 0 of that chunk, so
the alignment
+ * must be relative to the containing chunk rather than a global multiple
+ * of bytesPerChecksum.
+ * Returns the offset of {@code blockOffset} relative to the start of the
+ * chunk that contains it.
+ */
+ static long getChunkRelativeOffset(long blockOffset,
List<ContainerProtos.ChunkInfo> chunkInfos) {
+ long currentChunkOffset = 0;
+ for (ContainerProtos.ChunkInfo chunk : chunkInfos) {
+ long chunkEnd = currentChunkOffset + chunk.getLen();
+ if (blockOffset < chunkEnd) {
+ return blockOffset - currentChunkOffset;
+ }
+ currentChunkOffset = chunkEnd;
+ }
+ return blockOffset;
Review Comment:
This is out-of-range case is not supposed to happen. So, it should throw an
exception:
```java
throw new IllegalStateException("blockOffset " + blockOffset + " is out
of bounds: " + chunkInfos);
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2353,12 +2353,13 @@ private long readBlockImpl(ContainerCommandRequestProto
request, RandomAccessFil
} else {
bytesPerChecksum =
chunkInfos.get(0).getChecksumData().getBytesPerChecksum();
}
- // 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.
- // If the checksum type is NONE, we don't have to do this, but using no
checksums should be rare in practice and
- // it simplifies the code to always do this.
- final long offsetAlignment = readBlock.getOffset() % bytesPerChecksum;
+ long offsetAlignment;
+ if (checksumType != ContainerProtos.ChecksumType.NONE) {
+ final long chunkRelativeOffset =
getChunkRelativeOffset(readBlock.getOffset(), chunkInfos);
+ offsetAlignment = chunkRelativeOffset % bytesPerChecksum;
+ } else {
+ offsetAlignment = readBlock.getOffset() % bytesPerChecksum;
+ }
Review Comment:
Even for ChecksumType.NONE, it should use getChunkRelativeOffset(..).
```java
final long offsetAlignment =
getChunkRelativeOffset(readBlock.getOffset(), chunkInfos) % bytesPerChecksum;
long adjustedOffset = readBlock.getOffset() - offsetAlignment;
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2381,16 +2382,17 @@ private long readBlockImpl(ContainerCommandRequestProto
request, RandomAccessFil
if (checksumType != ContainerProtos.ChecksumType.NONE) {
final List<ByteString> checksums = getChecksums(adjustedOffset,
readLength,
- bytesPerChunk, bytesPerChecksum, chunkInfos);
+ bytesPerChecksum, chunkInfos);
LOG.debug("Read {} at adjustedOffset {}, readLength {}, bytesPerChunk
{}, bytesPerChecksum {}",
readBlock, adjustedOffset, readLength, bytesPerChunk,
bytesPerChecksum);
Review Comment:
Make it a single line and remove bytesPerChunk.
```diff
@@ -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;
```
```java
final List<ByteString> checksums = getChecksums(adjustedOffset,
readLength, bytesPerChecksum, chunkInfos);
LOG.debug("Read {} at adjustedOffset {}, readLength {},
bytesPerChecksum {}",
readBlock, adjustedOffset, readLength, bytesPerChecksum);
```
--
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]
