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: [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]

Reply via email to