[GitHub] [ozone] bshashikant commented on a change in pull request #1685: HDDS-4552. Read data from chunk into ByteBuffer[] instead of single ByteBuffer.

[GitBox <gi...@apache.org>]

bshashikant commented on a change in pull request #1685:
URL: https://github.com/apache/ozone/pull/1685#discussion_r546567517



##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java
##########
@@ -134,7 +135,12 @@ public synchronized void initialize() throws IOException {
     try {
       chunks = getChunkInfos();
     } catch (ContainerNotFoundException ioEx) {
-      refreshPipeline(ioEx);
+      LOG.info("Unable to read information for block {} from pipeline {}: {}." +

Review comment:
       LOG.info -> LOG.warn?

##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java
##########
@@ -301,36 +348,38 @@ private synchronized void readChunkFromContainer(int len) throws IOException {
       startByteIndex = chunkPosition;
     } else {
       // Start reading the chunk from the last chunkPosition onwards.
-      startByteIndex = bufferOffset + bufferLength;
+      startByteIndex = bufferOffsetWrtChunkData + bufferLength;
     }
 
-    // bufferOffset and bufferLength are updated below, but if read fails
+    // bufferOffsetWrtChunkData and bufferLength are updated after the data
+    // is read from Container and put into the buffers, but if read fails
     // and is retried, we need the previous position.  Position is reset after
     // successful read in adjustBufferPosition()
     storePosition();
 
+    long adjustedBuffersOffset, adjustedBuffersLen;
     if (verifyChecksum) {
-      // Update the bufferOffset and bufferLength as per the checksum
-      // boundary requirement.
-      computeChecksumBoundaries(startByteIndex, len);
+      // Adjust the chunk offset and length to include required checksum
+      // boundaries
+      Pair<Long, Long> adjustedOffsetAndLength =
+          computeChecksumBoundaries(startByteIndex, len);
+      adjustedBuffersOffset = adjustedOffsetAndLength.getLeft();
+      adjustedBuffersLen = adjustedOffsetAndLength.getRight();
     } else {
       // Read from the startByteIndex
-      bufferOffset = startByteIndex;
-      bufferLength = len;
+      adjustedBuffersOffset = startByteIndex;
+      adjustedBuffersLen = len;
     }
 
     // Adjust the chunkInfo so that only the required bytes are read from
     // the chunk.
     final ChunkInfo adjustedChunkInfo = ChunkInfo.newBuilder(chunkInfo)
-        .setOffset(bufferOffset + chunkInfo.getOffset())
-        .setLen(bufferLength)
+        .setOffset(adjustedBuffersOffset + chunkInfo.getOffset())
+        .setLen(adjustedBuffersLen)
         .build();
 
-    ByteString byteString = readChunk(adjustedChunkInfo);
-
-    buffers = byteString.asReadOnlyByteBufferList();
-    bufferIndex = 0;
-    allocated = true;
+    byte[] chunkData = readChunk(adjustedChunkInfo).toByteArray();

Review comment:
       This will do an additional buffer copy here. Let's see if we can explore anything here to avoid buffer copy here:
   https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/UnsafeByteOperations




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org