Implement write path zero copy by bf8086 · Pull Request #8526 · Alluxio/alluxio

bf8086 · 2019-03-06T01:56:46Z

Implemented write path zero copy to improve performance on remote and domain socket writes. Also made write handler write to disk asynchronously to avoid blocking network traffic.

AmplabJenkins · 2019-03-06T02:24:46Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-03-06T02:24:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/2331/
Test PASSed.

bf8086 · 2019-03-06T18:54:26Z

@calvinjia @gpang PTAL.

gpang

@bf8086 Thanks! I left a few questions.

gpang · 2019-03-06T21:32:00Z

core/common/src/main/java/alluxio/network/protocol/databuffer/ByteArrayDataBuffer.java


+  @Override
+  public void readBytes(OutputStream outputStream, int length) {
+    throw new UnsupportedOperationException("ByteArrayDataBuffer#readBytes is not implemented.");


Why are these not supported?

Can we distinguish the 2 calls from the exception messages?

This class is only used in unit tests and only one main function is implemented. It's not used in write path so I didn't spend any time completing this class.

Message updated.

If this is just a test class, then can we move it to a test directory? It is not obvious that this is only used for tests.

gpang · 2019-03-06T21:36:27Z

core/common/src/main/java/alluxio/worker/block/io/BlockWriter.java

+  /**
+   * Appends buffer.readableBytes() bytes to the end of this block writer from the given buffer.
+   *
+   * @param buffer the byte buffer to hold the data


is this the buffer that holds the data currently? to hold the data makes is sound like this is the buffer that is the output?

Yeah I copied the javadoc from an overloaded method. Updated.

gpang · 2019-03-06T22:31:40Z

core/client/fs/src/main/java/alluxio/client/block/stream/GrpcDataMessageBlockingStream.java

      return null;
    }
-    DataBuffer buffer = mMarshaller.pollBuffer(response);
+    DataBuffer buffer = mResponseMarshaller.pollBuffer(response);


mResponseMarshaller can be null?

User should not call this method if they do not have a response marshaller. I will add a check.

gpang · 2019-03-06T22:34:17Z

core/common/src/main/java/alluxio/conf/PropertyKey.java

+      new Builder(Name.WORKER_NETWORK_WRITER_BUFFER_SIZE_MESSAGES)
+          .setDefaultValue(8)
+          .setDescription("When a client writes to a remote worker, the maximum number of "
+              + "data messages to buffer by the server.")


this is what the worker buffers, per client?

Updated the description.

gpang · 2019-03-06T22:35:27Z

core/common/src/main/java/alluxio/grpc/GrpcSerializationUtils.java

+   * Gets a Netty buffer directly from a gRPC ReadableBuffer.
+   *
+   * @param buffer the input buffer
+   * @return the raw ByteBuf


when will this return null?

Updated javadoc.

gpang · 2019-03-06T22:39:26Z

core/client/fs/src/main/java/alluxio/client/block/stream/DefaultBlockWorkerClient.java

+      DataMessageMarshaller<WriteRequest> marshaller =
+          ((DataMessageMarshallerProvider<WriteRequest, WriteResponse>) responseObserver)
+              .getRequestMarshaller();
+      Preconditions.checkNotNull(marshaller);


Doesn't a DataMessageMarshallerProvider provide null sometimes?

DataMessageMarshallerProvider is a generic holder of request marshaller and response marshaller. Depending on the situation, the caller most likely only provide one of the marshallers. In this writeBlock method, it is required that the user should provide the request marshaller for zero copy to work correctly, hence the check.

If a user does not want to use zero copy at all, they should not pass in a DataMessageMarshallerProvider.

gpang · 2019-03-06T22:39:58Z

core/client/fs/src/main/java/alluxio/client/block/stream/DefaultBlockWorkerClient.java

-          ((DataMessageMarshallerProvider<ReadResponse>) responseObserver).getMarshaller();
+          ((DataMessageMarshallerProvider<ReadRequest, ReadResponse>) responseObserver)
+              .getResponseMarshaller();
+      Preconditions.checkNotNull(marshaller);


can't this be null sometimes?

Similar to above, if user does not want to use zero copy for reading blocks, they should not pass DataMessageMarshallerProvider instead of giving a null response marshaller.

gpang · 2019-03-06T23:34:19Z

core/common/src/main/java/alluxio/grpc/WriteRequestMarshaller.java

+      chunkBuffer = new NettyDataBuffer(
+          Unpooled.wrappedBuffer(message.getChunk().getData().asReadOnlyByteBuffer()));
+    }
+    int size = message.getSerializedSize() - chunkBuffer.readableBytes();


NIT: is this header size?

gpang · 2019-03-06T23:35:42Z

core/server/worker/src/main/java/alluxio/worker/grpc/AbstractWriteHandler.java

  private static final Logger LOG = LoggerFactory.getLogger(AbstractWriteHandler.class);

  private final StreamObserver<WriteResponse> mResponseObserver;
+  private final SerializingExecutor mSerializingExecutor;


can you add comments on these fields? I don't fully understand them.

bf8086 · 2019-03-07T01:36:33Z

@gpang Thanks for the review. I updated the PR to address your comments. PTAL.

AmplabJenkins · 2019-03-07T02:00:03Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-03-07T02:00:05Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/2367/
Test FAILed.

gpang · 2019-03-07T18:04:22Z

Jenkins, test this please

AmplabJenkins · 2019-03-07T18:26:30Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-03-07T18:26:32Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/2374/
Test PASSed.

gpang

@bf8086 I left a few comments.

gpang · 2019-03-07T18:22:16Z

core/server/worker/src/main/java/alluxio/worker/grpc/AbstractWriteHandler.java

   */
  AbstractWriteHandler(StreamObserver<WriteResponse> responseObserver) {
    mResponseObserver = responseObserver;
+    mSerializingExecutor = new SerializingExecutor(GrpcExecutors.BLOCK_WRITER_EXECUTOR);


the serializing executor only runs 1 task at a time, in order. Why does it need a large threadpool?

It only serialize tasks within the executor for a single request. There can be multiple concurrent requests which requires more threads for processing messages concurrently.

gpang · 2019-03-07T18:47:59Z

core/server/worker/src/main/java/alluxio/worker/grpc/AbstractWriteHandler.java

+    Preconditions.checkState(buffer.readableBytes() > 0,
+        "invalid data size from write request message");
+    try {
+      mSemaphore.acquire();


maybe there should be a helper method that does this acquire?

gpang · 2019-03-07T18:49:45Z

core/server/worker/src/main/java/alluxio/worker/grpc/AbstractWriteHandler.java

+      Thread.currentThread().interrupt();
+      return;
    }
+    mSerializingExecutor.execute(() -> {


I feel like every serializing executor should follow the model:

try { ... } finally { sem.release() }

Good catch. Fixed.

AmplabJenkins · 2019-03-07T19:32:15Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-03-07T19:32:16Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/2375/
Test PASSed.

bf8086 · 2019-03-07T20:35:55Z

Thanks @gpang! I updated the PR. PTAL.

bf8086 · 2019-03-08T19:18:46Z

@gpang I updated the PR to address your comment. PTAL.

AmplabJenkins · 2019-03-08T19:41:22Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-03-08T19:41:23Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/2385/
Test PASSed.

gpang

LGTM

calvinjia · 2019-03-12T01:13:20Z

core/server/worker/src/main/java/alluxio/worker/grpc/AbstractWriteHandler.java

+    if (!tryAcquireSemaphore()) {
+      return;
+    }
+    mSerializingExecutor.execute(() -> {


What will happen if client tries zero copy but the server does not support it?

The zero copy code path serialize and deserialize messages in the same format as the protobuf marshaller, therefore the client and the server can switch between zero copy or protobuf marshaller without breaking compatibility with the other side.

Implement write path zero copy

25c22b9

alluxio-bot assigned aaudiber Mar 6, 2019

gpang suggested changes Mar 6, 2019

View reviewed changes

Address comments

6c0c9a3

gpang reviewed Mar 7, 2019

View reviewed changes

Addressed comments

f21b56c

move ByteArrayDataBuffer

111d5a3

gpang approved these changes Mar 8, 2019

View reviewed changes

calvinjia reviewed Mar 12, 2019

View reviewed changes

calvinjia merged commit 99ef219 into Alluxio:master Mar 13, 2019

kaijchen mentioned this pull request Mar 3, 2023

RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS apache/ratis#834

Merged

Conversation

bf8086 commented Mar 6, 2019

Uh oh!

AmplabJenkins commented Mar 6, 2019

Uh oh!

AmplabJenkins commented Mar 6, 2019

Uh oh!

bf8086 commented Mar 6, 2019

Uh oh!

gpang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf8086 commented Mar 7, 2019

Uh oh!

AmplabJenkins commented Mar 7, 2019

Uh oh!

AmplabJenkins commented Mar 7, 2019

Uh oh!

gpang commented Mar 7, 2019

Uh oh!

AmplabJenkins commented Mar 7, 2019

Uh oh!

AmplabJenkins commented Mar 7, 2019

Uh oh!

gpang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment