szetszwo commented on code in PR #10764:
URL: https://github.com/apache/ozone/pull/10764#discussion_r3919777573
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2355,32 +2354,42 @@ 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;
- if (checksumType == ContainerProtos.ChecksumType.NONE) {
- checksumData = new ChecksumData(checksumType, 0);
- } else {
+ if (checksumType != ContainerProtos.ChecksumType.NONE) {
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 adjustedOffset = readBlock.getOffset() - offsetAlignment;
+ // TODO: Support client-side flag to toggle checksum verification.
+ // If checksum is disabled, chunk offset adjustment can be skipped.
+ final ChecksumBoundaries checksumBoundaries =
getChecksumBoundaries(readBlock.getOffset(),
+ readBlock.getLength(), chunkInfos, bytesPerChecksum);
+ long adjustedOffset = checksumBoundaries.offset;
+ long adjustLength = checksumBoundaries.length;
+ int chunkIndex = checksumBoundaries.startIndex;
+
+ ChecksumData checksumData = new ChecksumData(checksumType,
bytesPerChecksum);
final ByteBuffer buffer = ByteBuffer.allocate(responseDataSize);
blockFile.position(adjustedOffset);
long totalDataLength = 0;
int numResponses = 0;
- final long rounded = roundUp(readBlock.getLength() + offsetAlignment,
bytesPerChecksum);
- final long requiredLength = Math.min(rounded, blockData.getSize() -
adjustedOffset);
+ final long requiredLength = Math.min(adjustLength, blockData.getSize() -
adjustedOffset);
LOG.debug("adjustedOffset {}, requiredLength {}, blockSize {}",
adjustedOffset, requiredLength, blockData.getSize());
for (boolean shouldRead = true; totalDataLength < requiredLength &&
shouldRead;) {
+
+ int bufferLimit = (int) Math.min(responseDataSize, requiredLength -
totalDataLength);
+ final ContainerProtos.ChunkInfo nextChunk = chunkInfos.get(
+ searchChunkByOffset(adjustedOffset + bufferLimit, chunkInfos));
+
+ if (bufferLimit < requiredLength - totalDataLength) {
+ // bytesPerChecksum must be a power of 2.
+ bufferLimit = (int) (((bufferLimit - (nextChunk.getOffset() -
adjustedOffset)) & -((long) bytesPerChecksum))
+ + (nextChunk.getOffset() - adjustedOffset));
+ }
Review Comment:
Sorry that I don't understand these few lines.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2445,6 +2462,64 @@ static List<ByteString> getChecksums(long blockOffset,
int readLength, int bytes
return checksums;
}
+ /**
+ * 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.
+ * Returns the checksum boundaries of {@code ChecksumBoundaries} relative to
blockOffset and blockLength.
+ */
+ private static ChecksumBoundaries getChecksumBoundaries(long blockOffset,
long blockLength,
+ List<ContainerProtos.ChunkInfo> chunkInfos, long bytesPerChecksum) {
+ final int offsetChunkIndex = searchChunkByOffset(blockOffset, chunkInfos);
+ final long offsetAlignment = (blockOffset -
chunkInfos.get(offsetChunkIndex).getOffset()) % bytesPerChecksum;
+ final long adjustedOffset = blockOffset - offsetAlignment;
+ final long blockEnd = blockOffset + blockLength - 1;
+ final ContainerProtos.ChunkInfo lastChunk =
chunkInfos.get(searchChunkByOffset(blockEnd, chunkInfos));
+
+ final long chunkOffset = lastChunk.getOffset();
+ final long chunkLength = Math.min(
+ (getEndChecksumIndex(blockEnd, chunkOffset, bytesPerChecksum) + 1) *
bytesPerChecksum, lastChunk.getLen());
Review Comment:
Math.min should not be required -- we should always have adjusted length <=
lastChunk.getLen(). Change it to an assertion.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2445,6 +2462,64 @@ static List<ByteString> getChecksums(long blockOffset,
int readLength, int bytes
return checksums;
}
+ /**
+ * 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.
+ * Returns the checksum boundaries of {@code ChecksumBoundaries} relative to
blockOffset and blockLength.
+ */
+ private static ChecksumBoundaries getChecksumBoundaries(long blockOffset,
long blockLength,
+ List<ContainerProtos.ChunkInfo> chunkInfos, long bytesPerChecksum) {
+ final int offsetChunkIndex = searchChunkByOffset(blockOffset, chunkInfos);
+ final long offsetAlignment = (blockOffset -
chunkInfos.get(offsetChunkIndex).getOffset()) % bytesPerChecksum;
+ final long adjustedOffset = blockOffset - offsetAlignment;
+ final long blockEnd = blockOffset + blockLength - 1;
+ final ContainerProtos.ChunkInfo lastChunk =
chunkInfos.get(searchChunkByOffset(blockEnd, chunkInfos));
+
+ final long chunkOffset = lastChunk.getOffset();
+ final long chunkLength = Math.min(
+ (getEndChecksumIndex(blockEnd, chunkOffset, bytesPerChecksum) + 1) *
bytesPerChecksum, lastChunk.getLen());
+ return new ChecksumBoundaries(offsetChunkIndex, adjustedOffset,
+ chunkOffset + chunkLength - adjustedOffset);
+ }
+
+ private static int getEndChecksumIndex(long blockEnd, long chunkOffset, long
bytesPerChecksum) {
+ return (int) ((blockEnd - chunkOffset) / bytesPerChecksum);
+ }
+
+ private static final class ChecksumBoundaries {
+ private final int startIndex;
+ private final long offset;
+ private final long length;
+
+ private ChecksumBoundaries(int startIndex, long offset, long length) {
+ this.startIndex = startIndex;
+ this.offset = offset;
+ this.length = length;
+ }
+ }
+
+ private static int searchChunkByOffset(
+ long targetOffset,
+ List<ContainerProtos.ChunkInfo> chunkInfoList) {
+
+ int low = 0;
+ int high = chunkInfoList.size() - 1;
+
+ while (low <= high) {
+ int mid = (low + high) >>> 1;
+ long midVal = chunkInfoList.get(mid).getOffset();
+
+ if (midVal <= targetOffset) {
+ low = mid + 1;
+ } else {
+ high = mid - 1;
+ }
+ }
+
+ return high;
Review Comment:
Let's add some assertions. BTW, let's add javadoc and rename `targetOffset`
to `targetPosition`.
```java
/** @return the index of the chunk containing the target position. */
private static int searchChunk(long targetPosition,
List<ContainerProtos.ChunkInfo> chunkInfoList) {
int low = 0;
int high = chunkInfoList.size() - 1;
while (low <= high) {
int mid = (low + high) >>> 1;
long midVal = chunkInfoList.get(mid).getOffset();
if (midVal <= targetPosition) {
low = mid + 1;
} else {
high = mid - 1;
}
}
final ContainerProtos.ChunkInfo chunk = chunkInfoList.get(high);
Preconditions.checkState(targetPosition >= chunk.getOffset());
Preconditions.checkState(targetPosition < chunk.getOffset() +
chunk.getLen());
return high;
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2445,6 +2462,64 @@ static List<ByteString> getChecksums(long blockOffset,
int readLength, int bytes
return checksums;
}
+ /**
+ * 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.
+ * Returns the checksum boundaries of {@code ChecksumBoundaries} relative to
blockOffset and blockLength.
+ */
+ private static ChecksumBoundaries getChecksumBoundaries(long blockOffset,
long blockLength,
+ List<ContainerProtos.ChunkInfo> chunkInfos, long bytesPerChecksum) {
+ final int offsetChunkIndex = searchChunkByOffset(blockOffset, chunkInfos);
+ final long offsetAlignment = (blockOffset -
chunkInfos.get(offsetChunkIndex).getOffset()) % bytesPerChecksum;
+ final long adjustedOffset = blockOffset - offsetAlignment;
+ final long blockEnd = blockOffset + blockLength - 1;
+ final ContainerProtos.ChunkInfo lastChunk =
chunkInfos.get(searchChunkByOffset(blockEnd, chunkInfos));
+
+ final long chunkOffset = lastChunk.getOffset();
+ final long chunkLength = Math.min(
+ (getEndChecksumIndex(blockEnd, chunkOffset, bytesPerChecksum) + 1) *
bytesPerChecksum, lastChunk.getLen());
+ return new ChecksumBoundaries(offsetChunkIndex, adjustedOffset,
+ chunkOffset + chunkLength - adjustedOffset);
+ }
+
+ private static int getEndChecksumIndex(long blockEnd, long chunkOffset, long
bytesPerChecksum) {
+ return (int) ((blockEnd - chunkOffset) / bytesPerChecksum);
Review Comment:
blockEnd is inclusive (included the last byte) so getEndChecksumIndex should
use
- (blockEnd + 1 - chunkOffset) / bytesPerChecksum
I suggest to make blockEnd exclusive (The computation usual is simpler.
That why most Java API use inclusive begin and exclusive end.)
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2445,6 +2462,64 @@ static List<ByteString> getChecksums(long blockOffset,
int readLength, int bytes
return checksums;
}
+ /**
+ * 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.
+ * Returns the checksum boundaries of {@code ChecksumBoundaries} relative to
blockOffset and blockLength.
+ */
+ private static ChecksumBoundaries getChecksumBoundaries(long blockOffset,
long blockLength,
Review Comment:
Let's rename `blockOffset` and `blockLength` to `readOffset` and
`readLength`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2445,6 +2462,64 @@ static List<ByteString> getChecksums(long blockOffset,
int readLength, int bytes
return checksums;
}
+ /**
+ * 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.
+ * Returns the checksum boundaries of {@code ChecksumBoundaries} relative to
blockOffset and blockLength.
+ */
+ private static ChecksumBoundaries getChecksumBoundaries(long blockOffset,
long blockLength,
+ List<ContainerProtos.ChunkInfo> chunkInfos, long bytesPerChecksum) {
+ final int offsetChunkIndex = searchChunkByOffset(blockOffset, chunkInfos);
+ final long offsetAlignment = (blockOffset -
chunkInfos.get(offsetChunkIndex).getOffset()) % bytesPerChecksum;
+ final long adjustedOffset = blockOffset - offsetAlignment;
+ final long blockEnd = blockOffset + blockLength - 1;
+ final ContainerProtos.ChunkInfo lastChunk =
chunkInfos.get(searchChunkByOffset(blockEnd, chunkInfos));
+
+ final long chunkOffset = lastChunk.getOffset();
+ final long chunkLength = Math.min(
+ (getEndChecksumIndex(blockEnd, chunkOffset, bytesPerChecksum) + 1) *
bytesPerChecksum, lastChunk.getLen());
+ return new ChecksumBoundaries(offsetChunkIndex, adjustedOffset,
+ chunkOffset + chunkLength - adjustedOffset);
+ }
+
+ private static int getEndChecksumIndex(long blockEnd, long chunkOffset, long
bytesPerChecksum) {
+ return (int) ((blockEnd - chunkOffset) / bytesPerChecksum);
+ }
+
+ private static final class ChecksumBoundaries {
Review Comment:
This class is not really useful since it is only used once. We may inline
the methods:
```java
```
//readBlockImpl
// Align the read to checksum boundaries.
final int firstChunkIndex = searchChunk(readOffset, chunkInfos);
final int lastChunkIndex = searchChunk(readEnd - 1, chunkInfos);
final long adjustedOffset = adjustOffset(readOffset, bytesPerChecksum,
chunkInfos.get(firstChunkIndex).getOffset());
final long adjustedEnd = adjustEnd(readEnd, bytesPerChecksum,
chunkInfos.get(lastChunkIndex).getOffset());
final long adjustedLength = adjustedEnd - adjustedOffset;
```
```java
/**
* Adjust readOffset to algin with chunkOffset and bytesPerChecksum such
that
* (1) adjustedReadOffset <= readOffset, and
* (2) (adjustedReadOffset - chunkOffset) is a multiple of
bytesPerChecksum.
*
* @param readOffset inclusive
* @param chunkOffset inclusive
* @return the adjusted read offset (inclusive)
*/
static long adjustOffset(long readOffset, int bytesPerChecksum, long
chunkOffset) {
Preconditions.checkState(chunkOffset <= readOffset);
final long alignment = (readOffset - chunkOffset) % bytesPerChecksum;
return readOffset - alignment;
}
/**
* Adjust readEnd to algin with chunkOffset and bytesPerChecksum such that
* (1) adjustedReadEnd >= readEnd, and
* (2) (adjustedReadEnd - chunkOffset) is a multiple of bytesPerChecksum.
*
* @param readEnd exclusive
* @param chunkOffset inclusive
* @return the adjusted read end (exclusive)
*/
static long adjustEnd(long readEnd, int bytesPerChecksum, long
chunkOffset) {
Preconditions.checkState(chunkOffset < readEnd);
final long remainder = (readEnd - chunkOffset) % bytesPerChecksum;
final long alignment = remainder == 0 ? 0 : bytesPerChecksum - remainder;
return readEnd + alignment;
}
```
--
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]
