Implement zero copy for gRPC read path #8353

bf8086 · 2019-02-06T09:46:54Z

Added APIs for intercepting gRPC client and server with custom data serialization/deserialization.

Also made the worker send read response asynchronously to improve throughput for single read request.

bf8086 · 2019-02-06T09:47:21Z

AmplabJenkins · 2019-02-06T09:53:32Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-02-06T09:53:33Z

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/1913/
Test FAILed.

AmplabJenkins · 2019-02-06T19:07:59Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-02-06T19:08:00Z

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/1916/
Test PASSed.

gpang · 2019-02-06T21:45:07Z

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

+  @Override
+  public void beforeStart(ClientCallStreamObserver<ReqT> requestStream) {
+    if (mObserver instanceof ClientResponseObserver) {
+      ((ClientResponseObserver<ReqT, RespT>) mObserver).beforeStart(requestStream);


why is beforeStart called selectively?

This is just to make sure the observer provided actually implemented the interface for beforeStart(). I can put a warning message if it is not implemented.

gpang · 2019-02-06T21:49:15Z

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

-    return mStreamingAsyncStub.readBlock(responseObserver);
+  public StreamObserver<ReadRequest> readBlock(
+      StreamObserver<ReadResponse> responseObserver) {
+    if (responseObserver instanceof DataMessageClientResponseObserver) {


Would it make sense to have a new interface with just getMarshaller, and just check/cast to that interface when getting the marshaller?

gpang · 2019-02-06T21:50:42Z

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

  private final WorkerNetAddress mAddress;

  private final GrpcBlockingStream<ReadRequest, ReadResponse> mStream;
+  private final ReadResponseMarshaller mMarhshaller;


gpang · 2019-02-06T21:52:32Z

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

+  public static final PropertyKey WORKER_NETWORK_ZEROCOPY_ENABLED =
+      new Builder(Name.WORKER_NETWORK_ZEROCOPY_ENABLED)
+          .setDefaultValue(true)
+          .setDescription("Whether zero copy is enabled on worker when processing data streams.")


is this for reading data from streams or writing data to streams?

This is a general switch for all worker side zero-copy code path. For now only read path is implemented.

gpang · 2019-02-06T21:52:45Z

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

+  public static final PropertyKey USER_NETWORK_ZEROCOPY_ENABLED =
+      new Builder(Name.USER_NETWORK_ZEROCOPY_ENABLED)
+          .setDefaultValue(true)
+          .setDescription("Whether zero copy is enabled on client when processing data streams.")


is this for reading data from streams or writing data to streams?

This is a general switch for all client side zero-copy code path. For now only read path is implemented.

gpang · 2019-02-06T21:55:52Z

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

+
+    try {
+      sNettyWritableBufferConstruct = getPrivateConstructor(
+          "io.grpc.netty.NettyWritableBuffer", ByteBuf.class);


Can these strings be constants?

AmplabJenkins · 2019-02-07T00:48:33Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-02-07T00:48:34Z

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/1926/
Test PASSed.

AmplabJenkins · 2019-02-07T00:57:09Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-02-07T00:57:10Z

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/1927/
Test PASSed.

calvinjia · 2019-02-07T01:36:38Z

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

+    if (mObserver instanceof ClientResponseObserver) {
+      ((ClientResponseObserver<ReqT, RespT>) mObserver).beforeStart(requestStream);
+    } else {
+      LOG.warn("observer does not implement ClientResponseObserver:beforeStart");


Do you want to print out the class for the mObserver?

2 comments.

will this be spammy?

add the mObserver to the log message.

This will not be spammy unless someone pass in the wrong observer. In most use cases the DataMessageClientResponseObserver is a wrapper of the original stream observer user implements, so user should only call method on the wrapper that the original observer actually implements.

calvinjia · 2019-02-07T01:39:59Z

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

-  public StreamObserver<ReadRequest> readBlock(StreamObserver<ReadResponse> responseObserver) {
-    return mStreamingAsyncStub.readBlock(responseObserver);
+  public StreamObserver<ReadRequest> readBlock(
+      StreamObserver<ReadResponse> responseObserver) {


Can this be on one line?

Reformatted.

gpang · 2019-02-07T15:14:52Z

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

+    if (mObserver instanceof ClientResponseObserver) {
+      ((ClientResponseObserver<ReqT, RespT>) mObserver).beforeStart(requestStream);
+    } else {
+      LOG.warn("observer does not implement ClientResponseObserver:beforeStart");


2 comments.

will this be spammy?

add the mObserver to the log message.

gpang · 2019-02-07T15:32:03Z

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

+   */
+  public abstract T combineData(DataMessage<T, DataBuffer> message);
+
+  protected abstract ByteBuf[] extractMessageBuffer(T message) throws IOException;


can you javadoc these methods? I'm not sure what they are supposed to do.

I renamed the methods and added some java docs. Hopefully now they are easier to understand.

AmplabJenkins · 2019-02-07T20:00:57Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-02-07T20:00:58Z

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/1942/
Test PASSed.

bf8086 · 2019-02-07T21:14:44Z

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

gpang

LGTM

add zero copy for gRPC read path

ea0da3c

fix checkstyles

7d17b95

alluxio-bot assigned calvinjia Feb 6, 2019

gpang suggested changes Feb 6, 2019

View reviewed changes

bf8086 added 2 commits February 6, 2019 16:28

Address comments

fb3abf4

fix buffer copy

7fff931

calvinjia reviewed Feb 7, 2019

View reviewed changes

gpang reviewed Feb 7, 2019

View reviewed changes

addressed comments

fcad257

gpang approved these changes Feb 7, 2019

View reviewed changes

calvinjia merged commit 06d86b4 into Alluxio:master Feb 7, 2019

kaijchen mentioned this pull request Mar 3, 2023

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

Merged

Implement zero copy for gRPC read path #8353

Implement zero copy for gRPC read path #8353

Uh oh!

Conversation

bf8086 commented Feb 6, 2019

Uh oh!

bf8086 commented Feb 6, 2019

Uh oh!

AmplabJenkins commented Feb 6, 2019

Uh oh!

AmplabJenkins commented Feb 6, 2019

Uh oh!

AmplabJenkins commented Feb 6, 2019

Uh oh!

AmplabJenkins commented Feb 6, 2019

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 Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AmplabJenkins commented Feb 7, 2019

Uh oh!

AmplabJenkins commented Feb 7, 2019

Uh oh!

AmplabJenkins commented Feb 7, 2019

Uh oh!

AmplabJenkins commented Feb 7, 2019

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!

AmplabJenkins commented Feb 7, 2019

Uh oh!

AmplabJenkins commented Feb 7, 2019

Uh oh!

bf8086 commented Feb 7, 2019

Uh oh!

gpang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

bf8086 Feb 6, 2019 •

edited

Loading