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