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

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

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



##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java
##########
@@ -364,7 +404,18 @@ protected ByteString readChunk(ChunkInfo readChunkInfo) throws IOException {
       throw new IOException("Unexpected OzoneException: " + e.toString(), e);
     }
 
-    return readChunkResponse.getData();
+    if (readChunkResponse.hasData()) {
+      return readChunkResponse.getData().asReadOnlyByteBufferList();
+    } else if (readChunkResponse.hasDataBuffers()) {
+      List<ByteString> buffersList = readChunkResponse.getDataBuffers()
+          .getBuffersList();
+      return buffersList.stream()
+          .map(ByteString::asReadOnlyByteBuffer)
+          .collect(Collectors.toList());

Review comment:
       I think we should avoid streams on read/write path.  Earlier these were found to cause CPU usage hotspots.  See eg. HDDS-3702.
   
   (Also in few other instances below.)

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -222,7 +228,33 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     }
 
     long len = info.getLen();
-    ByteBuffer data = ByteBuffer.allocate((int) len);
+
+    long bufferCapacity = 0;
+    if (info.isReadDataIntoSingleBuffer()) {
+      // Older client - read all chunk data into one single buffer.
+      bufferCapacity = len;
+    } else {
+      // Set buffer capacity to checksum boundary size so that each buffer
+      // corresponds to one checksum. If checksum is NONE, then set buffer
+      // capacity to default (OZONE_CHUNK_READ_BUFFER_DEFAULT_SIZE_KEY = 64KB).
+      ChecksumData checksumData = info.getChecksumData();
+
+      if (checksumData != null) {
+        if (checksumData.getChecksumType() ==
+            ContainerProtos.ChecksumType.NONE) {
+          bufferCapacity = defaultReadBufferCapacity;
+        } else {
+          bufferCapacity = checksumData.getBytesPerChecksum();
+        }
+      }
+    }
+    // If the buffer capacity is 0, set all the data into one ByteBuffer
+    if (bufferCapacity == 0) {
+      bufferCapacity = len;
+    }

Review comment:
       This block seems to be duplicated from `FilePerBlock...`.  Can it be extracted?

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -255,16 +287,16 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
         if (file.exists()) {
           long offset = info.getOffset() - chunkFileOffset;
           Preconditions.checkState(offset >= 0);
-          ChunkUtils.readData(file, data, offset, len, volumeIOStats);
-          return ChunkBuffer.wrap(data);
+          ChunkUtils.readData(file, dataBuffers, offset, len, volumeIOStats);
+          return ChunkBuffer.wrap(Lists.newArrayList(dataBuffers));
         }
       } catch (StorageContainerException ex) {
         //UNABLE TO FIND chunk is not a problem as we will try with the
         //next possible location
         if (ex.getResult() != UNABLE_TO_FIND_CHUNK) {
           throw ex;
         }
-        data.clear();
+        dataBuffers = null;

Review comment:
       If read from first location fails and we have to fall back to the temp chunk file, this would cause exception.




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