[GitHub] [ozone] fapifta commented on a change in pull request #2767: Hdds 5491: EC: Write should handle node failures.

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

fapifta commented on a change in pull request #2767:
URL: https://github.com/apache/ozone/pull/2767#discussion_r741499089



##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java
##########
@@ -700,6 +714,7 @@ void writeChunkToContainer(ChunkBuffer chunk) throws IOException {
       handleInterruptedException(ex, false);
     }
     containerBlockData.addChunks(chunkInfo);
+    return null;

Review comment:
       If we are changing the signature of this method, shouldn't we also return the future inside the try block for future users of this method whom might expect a non-null value on success?
   
   I noticed that we are changing the method just to store the future object in ECBlockOutputStream, as we override the method there, why not just store the variable in the overridden method into a private field of ECBlockOutputStream?
   
   As an alternative if this seems to be useful to store and return, why not store here in the BlockOutputStream, and provide an accessor method for the future from here?

##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ECBlockOutputStream.java
##########
@@ -35,12 +38,15 @@
 import java.util.concurrent.ExecutionException;
 
 import static org.apache.hadoop.hdds.scm.storage.ContainerProtocolCalls.putBlockAsync;
+import static org.apache.hadoop.hdds.scm.storage.ContainerProtocolCalls.writeChunkAsync;
 
 /**
  * Handles the chunk EC writes for an EC internal block.
  */
 public class ECBlockOutputStream extends BlockOutputStream{
 
+  private CompletableFuture<ContainerProtos.ContainerCommandResponseProto>
+      currentChunkRspFuture = null;

Review comment:
       This variable is set from two places, from the write method, and from executePutBlock, executePutBlock happens on flush, and close, which is a possible race.
   With the current synchronous grpc client it is not a problem, but if we switch to really async writes in the grpc client, then this might cause a race, and we might miss to check some of the futures. Is there a guarantee that I miss here?

##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ECBlockOutputStream.java
##########
@@ -63,7 +69,65 @@ public ECBlockOutputStream(
 
   @Override
   public void write(byte[] b, int off, int len) throws IOException {
-    writeChunkToContainer(ChunkBuffer.wrap(ByteBuffer.wrap(b, off, len)));
+    this.currentChunkRspFuture =
+        writeChunkToContainer(ChunkBuffer.wrap(ByteBuffer.wrap(b, off, len)));
+  }
+
+  /**
+   * Writes buffered data as a new chunk to the container and saves chunk
+   * information to be used later in putKey call.
+   *
+   * @throws IOException if there is an I/O error while performing the call
+   * @throws OzoneChecksumException if there is an error while computing
+   * checksum
+   * @return ContainerCommandResponseProto
+   */
+  CompletableFuture<ContainerProtos.ContainerCommandResponseProto>
+      writeChunkToContainer(ChunkBuffer chunk) throws IOException {

Review comment:
       We are duplicating the bulk of this method from BlockOutputStream, wouldn't it be better to skip adding the accessor methods for private variables of BlockOutputStream, and extract the part that is different to a method that we can override here?
   
   As I see the major change is the point where we are adding the chunkInfo objects to the containerBlockData builder used in putBlock. I am not sure, but I guess we can change the place of addChunkInfo call in the original method, at least I do not see a reason why a failed write should have that call, and with that and with returning the future in the original method we do not need this override, and the getters in the super class at all. What do you think?




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