szetszwo commented on code in PR #10764:
URL: https://github.com/apache/ozone/pull/10764#discussion_r3942199439
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -57,7 +57,6 @@
import static
org.apache.hadoop.hdds.scm.protocolPB.ContainerCommandResponseBuilders.putBlockResponseSuccess;
import static
org.apache.hadoop.hdds.scm.protocolPB.ContainerCommandResponseBuilders.unsupportedRequest;
import static
org.apache.hadoop.hdds.scm.utils.ClientCommandsUtils.getReadChunkVersion;
-import static org.apache.hadoop.hdds.utils.IOUtils.roundUp;
Review Comment:
The roundUp method becomes unused. Let's remove it.
##########
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:
Thanks for the explanation. I got it now.
- searchChunkByOffset(adjustedOffset + bufferLimit, ..) should use
adjustedOffset + bufferLimit - 1 for the end position (inclusive)
- Let's call it endChunk.
- Since endChunk index is increasing, let's use linear search.
- Move the entire computation to a new class:
```java
static class BufferLimitComputation {
private final int responseDataSize;
private final int bitMask;
private final List<ContainerProtos.ChunkInfo> chunks;
private int chunkIndex;
private long lastPosition = 0;
BufferLimitComputation(int responseDataSize, int bytesPerChecksum,
List<ContainerProtos.ChunkInfo> chunks, int firstChunkIndex) {
this.responseDataSize = responseDataSize;
Preconditions.assertSame(1, Long.bitCount(bytesPerChecksum),
"bitCount" ); // check power of 2.
// Suppose bytesPerChecksum = 00001000 (a power of 2), then bitMask =
11111000.
// The following two computations are the same:
// We will use (n & bitMask) to compute ((n / bytesPerChecksum) *
bytesPerChecksum).
this.bitMask = -bytesPerChecksum;
this.chunks = chunks;
this.chunkIndex = firstChunkIndex;
}
int getChunkIndex() {
return chunkIndex;
}
/**
* @param position must be increasing in subsequent calls to this method.
* @return the chunk containing the given position.
*/
ContainerProtos.ChunkInfo findChunk(long position) {
Preconditions.assertTrue(position >= lastPosition);
lastPosition = position;
for(; chunkIndex < chunks.size(); chunkIndex++) {
final ContainerProtos.ChunkInfo chunk = chunks.get(chunkIndex);
if (position >= chunk.getOffset() && position < chunk.getOffset() +
chunk.getLen()) {
return chunk;
}
}
throw new IllegalStateException("Position " + position + " not found
in " + chunks);
}
int compute(long offset, long remainingLength) {
if (responseDataSize >= remainingLength) {
return Math.toIntExact(remainingLength);
}
final long end = offset + responseDataSize - 1; // inclusive
final ContainerProtos.ChunkInfo endChunk = findChunk(end);
final int lengthExcludingEndChunk =
Math.toIntExact(endChunk.getOffset() - offset);
// round down the length at endChunk
final int lengthAtEndChunk = (responseDataSize -
lengthExcludingEndChunk) & bitMask;
return lengthExcludingEndChunk + lengthAtEndChunk;
}
}
```
##########
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);
Review Comment:
Since we already have Math.min when adjusting the end, this should not be
required. Change it to an assertion.
--
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]
