[GitHub] [ozone] szetszwo commented on a change in pull request #2557: HDDS-5599. [Ozone-Streaming]drop BufferPool and ChunkBuffer to avoid buffer copying

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

szetszwo commented on a change in pull request #2557:
URL: https://github.com/apache/ozone/pull/2557#discussion_r693578999



##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/CommitWatcher.java
##########
@@ -84,6 +84,13 @@ public CommitWatcher(BufferPool bufferPool, XceiverClientSpi xceiverClient) {
     futureMap = new ConcurrentHashMap<>();
   }
 
+  public CommitWatcher(XceiverClientSpi xceiverClient) {

Review comment:
       Let's copy CommitWatcher to a new class, say StreamCommitWatcher?  In StreamCommitWatcher, we don't need bufferPool.  The commitIndex2flushedDataMap should be different -- it should not use ChunkBuffer anymore.

##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockDataStreamOutput.java
##########
@@ -501,38 +356,11 @@ private void watchForCommit(boolean bufferFull) throws IOException {
     return flushFuture;
   }
 
+  // TODO We are no longer using the bufferPool, so we do not need to flush the
+  //  data in it. We can discuss whether add other behaviors here.

Review comment:
       flush() should wait for all the futures to complete.  Move the code below 
   {code}
   //executePutBlock(..)
      try {
         CompletableFuture.allOf(futures.toArray(EMPTY_FUTURE_ARRAY)).get();
       } catch (Exception e) {
         LOG.warn("Failed to write all chunks through stream: " + e);
         throw new IOException(e);
       }
   {code}
   from executePutBlock(..) to flush().

##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockDataStreamOutput.java
##########
@@ -426,16 +294,6 @@ private void watchForCommit(boolean bufferFull) throws IOException {
       boolean force) throws IOException {
     checkOpen();
     long flushPos = totalDataFlushedLength;
-    final List<ChunkBuffer> byteBufferList;
-    if (!force) {
-      Preconditions.checkNotNull(bufferList);
-      byteBufferList = bufferList;
-      bufferList = null;
-      Preconditions.checkNotNull(byteBufferList);
-    } else {
-      byteBufferList = null;
-    }
-

Review comment:
       Move the try-catch below to flush() and then call flush() here.




-- 
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: issues-unsubscribe@ozone.apache.org

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