-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Implement write path zero copy #8526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ | |
| import alluxio.grpc.GrpcSerializationUtils; | ||
| import alluxio.util.network.NettyUtils; | ||
|
|
||
| import com.google.common.base.Preconditions; | ||
| import com.google.common.io.Closer; | ||
| import io.grpc.StatusRuntimeException; | ||
| import io.grpc.stub.StreamObserver; | ||
|
|
@@ -114,14 +115,29 @@ public void close() throws IOException { | |
|
|
||
| @Override | ||
| public StreamObserver<WriteRequest> writeBlock(StreamObserver<WriteResponse> responseObserver) { | ||
| return mStreamingAsyncStub.writeBlock(responseObserver); | ||
| if (responseObserver instanceof DataMessageMarshallerProvider) { | ||
| DataMessageMarshaller<WriteRequest> marshaller = | ||
| ((DataMessageMarshallerProvider<WriteRequest, WriteResponse>) responseObserver) | ||
| .getRequestMarshaller(); | ||
| Preconditions.checkNotNull(marshaller); | ||
| return mStreamingAsyncStub | ||
| .withOption(GrpcSerializationUtils.OVERRIDDEN_METHOD_DESCRIPTOR, | ||
| BlockWorkerGrpc.getWriteBlockMethod().toBuilder() | ||
| .setRequestMarshaller(marshaller) | ||
| .build()) | ||
| .writeBlock(responseObserver); | ||
| } else { | ||
| return mStreamingAsyncStub.writeBlock(responseObserver); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public StreamObserver<ReadRequest> readBlock(StreamObserver<ReadResponse> responseObserver) { | ||
| if (responseObserver instanceof DataMessageMarshallerProvider) { | ||
| DataMessageMarshaller<ReadResponse> marshaller = | ||
| ((DataMessageMarshallerProvider<ReadResponse>) responseObserver).getMarshaller(); | ||
| ((DataMessageMarshallerProvider<ReadRequest, ReadResponse>) responseObserver) | ||
| .getResponseMarshaller(); | ||
| Preconditions.checkNotNull(marshaller); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above, if user does not want to use zero copy for reading blocks, they should not pass |
||
| return mStreamingAsyncStub | ||
| .withOption(GrpcSerializationUtils.OVERRIDDEN_METHOD_DESCRIPTOR, | ||
| BlockWorkerGrpc.getReadBlockMethod().toBuilder() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| import alluxio.grpc.DataMessageMarshaller; | ||
| import alluxio.network.protocol.databuffer.DataBuffer; | ||
|
|
||
| import com.google.common.base.Preconditions; | ||
| import io.grpc.stub.StreamObserver; | ||
|
|
||
| import java.io.IOException; | ||
|
|
@@ -31,32 +32,40 @@ | |
| */ | ||
| @NotThreadSafe | ||
| public class GrpcDataMessageBlockingStream<ReqT, ResT> extends GrpcBlockingStream<ReqT, ResT> { | ||
| private final DataMessageMarshaller<ResT> mMarshaller; | ||
| private final DataMessageMarshaller<ReqT> mRequestMarshaller; | ||
| private final DataMessageMarshaller<ResT> mResponseMarshaller; | ||
|
|
||
| /** | ||
| * @param rpcFunc the gRPC bi-directional stream stub function | ||
| * @param bufferSize maximum number of incoming messages the buffer can hold | ||
| * @param description description of this stream | ||
| * @param deserializer custom deserializer for the response | ||
| * @param requestMarshaller the marshaller for the request | ||
| * @param responseMarshaller the marshaller for the response | ||
| */ | ||
| public GrpcDataMessageBlockingStream(Function<StreamObserver<ResT>, StreamObserver<ReqT>> rpcFunc, | ||
| int bufferSize, String description, DataMessageMarshaller<ResT> deserializer) { | ||
| int bufferSize, String description, DataMessageMarshaller<ReqT> requestMarshaller, | ||
| DataMessageMarshaller<ResT> responseMarshaller) { | ||
| super((resObserver) -> { | ||
| DataMessageClientResponseObserver<ReqT, ResT> newObserver = | ||
| new DataMessageClientResponseObserver<>(resObserver, deserializer); | ||
| new DataMessageClientResponseObserver<>(resObserver, requestMarshaller, | ||
| responseMarshaller); | ||
| StreamObserver<ReqT> requestObserver = rpcFunc.apply(newObserver); | ||
| return requestObserver; | ||
| }, bufferSize, description); | ||
| mMarshaller = deserializer; | ||
| mRequestMarshaller = requestMarshaller; | ||
| mResponseMarshaller = responseMarshaller; | ||
| } | ||
|
|
||
| @Override | ||
| public ResT receive(long timeoutMs) throws IOException { | ||
| if (mResponseMarshaller == null) { | ||
| return super.receive(timeoutMs); | ||
| } | ||
| DataMessage<ResT, DataBuffer> message = receiveDataMessage(timeoutMs); | ||
| if (message == null) { | ||
| return null; | ||
| } | ||
| return mMarshaller.combineData(message); | ||
| return mResponseMarshaller.combineData(message); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -70,16 +79,38 @@ public ResT receive(long timeoutMs) throws IOException { | |
| * @throws IOException if any error occurs | ||
| */ | ||
| public DataMessage<ResT, DataBuffer> receiveDataMessage(long timeoutMs) throws IOException { | ||
| Preconditions.checkNotNull(mResponseMarshaller, | ||
| "Cannot retrieve data message without a response marshaller."); | ||
| ResT response = super.receive(timeoutMs); | ||
| if (response == null) { | ||
| return null; | ||
| } | ||
| DataBuffer buffer = mMarshaller.pollBuffer(response); | ||
| DataBuffer buffer = mResponseMarshaller.pollBuffer(response); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User should not call this method if they do not have a response marshaller. I will add a check. |
||
| return new DataMessage<>(response, buffer); | ||
| } | ||
|
|
||
| /** | ||
| * Sends a request. Will wait until the stream is ready before sending or timeout if the | ||
| * given timeout is reached. | ||
| * | ||
| * @param message the request message with {@link DataBuffer attached} | ||
| * @param timeoutMs maximum wait time before throwing a {@link DeadlineExceededException} | ||
| * @throws IOException if any error occurs | ||
| */ | ||
| public void sendDataMessage(DataMessage<ReqT, DataBuffer> message, long timeoutMs) | ||
| throws IOException { | ||
| if (mRequestMarshaller != null) { | ||
| mRequestMarshaller.offerBuffer(message.getBuffer(), message.getMessage()); | ||
| } | ||
| super.send(message.getMessage(), timeoutMs); | ||
| } | ||
|
|
||
| @Override | ||
| public void waitForComplete(long timeoutMs) throws IOException { | ||
| if (mResponseMarshaller == null) { | ||
| super.waitForComplete(timeoutMs); | ||
| return; | ||
| } | ||
| DataMessage<ResT, DataBuffer> message; | ||
| while (!isCanceled() && (message = receiveDataMessage(timeoutMs)) != null) { | ||
| if (message.getBuffer() != null) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't a
DataMessageMarshallerProviderprovidenullsometimes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataMessageMarshallerProvideris a generic holder of request marshaller and response marshaller. Depending on the situation, the caller most likely only provide one of the marshallers. In thiswriteBlockmethod, 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.