Add an example of server side compression support #5358

AmiDavidW · 2019-02-13T20:46:57Z

I was looking for a way to enable the compression of server responses, but cannot find a direct answer, as mentioned here.

I figured out a way to get it working and would like to have some comments and feedbacks.

It would be great if we have an officially recommended way to implement it.

thelinuxfoundation · 2019-02-13T20:46:58Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @AmiDavidW isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

thelinuxfoundation · 2019-02-21T23:01:36Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @AmiDavidW isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldClient.java

examples/src/main/java/io/grpc/examples/experimental/CompressionServerInterceptor.java

ejona86 · 2019-02-26T20:42:02Z

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServer.java

+    server = ServerBuilder.forPort(port)
+        .compressorRegistry(CompressorRegistry.getDefaultInstance())
+        .decompressorRegistry(DecompressorRegistry.getDefaultInstance())
+        .intercept(new CompressionServerInterceptor())


It needs to be made clear why you are using an interceptor. Frequently individual methods that know they want compression could enable it by casting StreamObserver to ServerCallStreamObserver and calling setCompression("gzip").

I assume you're using an interceptor here to enable it for all methods.

Thanks a lot for the alternative. I could not find a recommended way to enable server response compression and figured out to use ServerInterceptor to do that.

Yes, I want to enable it in one place for all the methods.

I tried ServerCallStreamObserver.setCompression("gzip") and it works. I just have some questions:

Is it always safe to cast StreamObserver to ServerCallStreamObserver?

Can we do it in one place?

Do we need to do explicit check to see if the client accept/prefer compression? If yes, how?
That's related to one of my previous replies.

Thanks again.

Is it always safe to cast StreamObserver to ServerCallStreamObserver?

Yes, it is safe for the StreamObserver passed to the server by gRPC. It is really unfortunate that it is hard to discover and awkward and ugly, but it's what we were forced to do to 1) add features and 2) not break existing code.

Can we do it in one place?

To do it in one place, use the interceptor. If you want to configure it per-method, use the ServerCallStreamObserver.

Do we need to do explicit check to see if the client accept/prefer compression? If yes, how?

No. We do that in gRPC. Although it does seem the Javadoc should be improved to say it is safe.

Sent out #5401 to improve documentation.

Thanks a lot for the explanations and documentation improvement. Provide two examples to show both cases: PerMethod and AllMethods.

ejona86

Looks pretty good. A few nits, but most of what's left is just adding some documentation so its a bit more clear to users what they're looking at.

examples/src/main/java/io/grpc/examples/experimental/CompressionServerInterceptor.java

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServerPerMethod.java

ejona86 · 2019-02-27T22:34:14Z

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServerAllMethods.java

+import io.grpc.stub.StreamObserver;
+
+/**
+ * Server that manages startup/shutdown of a {@code Greeter} server.


Seems we need some more documentation. How about adding here: "Uses an interceptor to enable compression for all responses."

Updated as the review comment.

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServerAllMethods.java

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServerPerMethod.java

examples/src/main/java/io/grpc/examples/experimental/CompressionServerInterceptor.java

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServerAllMethods.java

examples/src/main/java/io/grpc/examples/experimental/CompressingHelloWorldServerPerMethod.java

examples/src/main/java/io/grpc/examples/experimental/CompressionServerInterceptor.java

thelinuxfoundation · 2019-03-01T00:01:21Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

One or more commits are not associated with a GitHub user. The email used in the Git commit must match a verified email address in a GitHub user profile. For more information, please see https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

Regards,
CLA GitHub bot

ejona86 · 2019-03-06T19:47:46Z

@carl-mastrangelo, this looks good for a follow-up review.

carl-mastrangelo

LGTM

carl-mastrangelo · 2019-03-06T19:52:05Z

@ejona86 I'll merge once the tests pass.

sanjaypujare · 2019-03-06T20:04:53Z

It will be really nice to have a README.md describing the example. I notice a new directory "experimental" is being created? We can have one readme for all the examples in this directory - but a readme will be highly desirable.

carl-mastrangelo · 2019-03-06T20:08:55Z

@sanjaypujare I agree, followup PR okay?

sanjaypujare · 2019-03-06T20:12:21Z

@carl-mastrangelo sounds good. Who will be contributing the PR?

carl-mastrangelo · 2019-03-06T20:25:35Z

@sanjaypujare Since you suggested it, I assume you have something in mind. Let's make sure not to overtax our contributors.

carl-mastrangelo · 2019-03-06T20:26:09Z

@AmiDavidW Merged, thanks!

AmiDavidW · 2019-03-07T23:50:18Z

@carl-mastrangelo @ejona86 Great! Thank you guys for the reviews to make it better!

AmiDavidW force-pushed the compression_example_server branch from bd10a4a to 43736b4 Compare February 21, 2019 23:01

Add server side compression support

9550a2d

AmiDavidW force-pushed the compression_example_server branch from 43736b4 to 9550a2d Compare February 26, 2019 16:50

AmiDavidW added 2 commits February 26, 2019 13:03

Fix import issues

42bf7ee

Formatted code

2c53160

carl-mastrangelo requested a review from zhangkun83 February 26, 2019 20:30

ejona86 reviewed Feb 26, 2019

View reviewed changes

AmiDavidW added 2 commits February 26, 2019 16:35

Remove the unnecessary configurations

6d21c28

Remove unnecessary configuration and simplify acceptance check

6e52bd1

ejona86 mentioned this pull request Feb 27, 2019

core,stub: Document grpc performs compression nego on server #5401

Merged

AmiDavidW added 3 commits February 27, 2019 17:05

Remove redundant check

1974a7f

Provide two examples: PerMethod and AllMethods

5a965f4

Class rename

ce3a21d

ejona86 reviewed Feb 27, 2019

View reviewed changes

examples/src/main/java/io/grpc/examples/experimental/CompressionServerInterceptor.java Outdated Show resolved Hide resolved

carl-mastrangelo reviewed Feb 27, 2019

View reviewed changes

AmiDavidW added 3 commits February 28, 2019 16:08

Use anonymous class instead of stand-alone class

85d1e62

Cast up-front to access all methods and make the type explicit

c9815c5

Update documentation

345103b

Clean up modifier

a111ab7

AmiDavidW force-pushed the compression_example_server branch from 907abf7 to a111ab7 Compare March 1, 2019 00:07

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 6, 2019

grpc-kokoro removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Mar 6, 2019

ejona86 approved these changes Mar 6, 2019

View reviewed changes

carl-mastrangelo approved these changes Mar 6, 2019

View reviewed changes

carl-mastrangelo merged commit 1284090 into grpc:master Mar 6, 2019

AmiDavidW deleted the compression_example_server branch March 7, 2019 23:52

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an example of server side compression support #5358

Add an example of server side compression support #5358

AmiDavidW commented Feb 13, 2019

thelinuxfoundation commented Feb 13, 2019

thelinuxfoundation commented Feb 21, 2019

ejona86 Feb 26, 2019

AmiDavidW Feb 26, 2019

ejona86 Feb 27, 2019

ejona86 Feb 27, 2019

AmiDavidW Feb 27, 2019

ejona86 left a comment

ejona86 Feb 27, 2019

AmiDavidW Feb 28, 2019

thelinuxfoundation commented Mar 1, 2019

ejona86 commented Mar 6, 2019

carl-mastrangelo left a comment

carl-mastrangelo commented Mar 6, 2019

sanjaypujare commented Mar 6, 2019

carl-mastrangelo commented Mar 6, 2019

sanjaypujare commented Mar 6, 2019

carl-mastrangelo commented Mar 6, 2019

carl-mastrangelo commented Mar 6, 2019

AmiDavidW commented Mar 7, 2019

Add an example of server side compression support #5358

Add an example of server side compression support #5358

Conversation

AmiDavidW commented Feb 13, 2019

thelinuxfoundation commented Feb 13, 2019

thelinuxfoundation commented Feb 21, 2019

ejona86 Feb 26, 2019

Choose a reason for hiding this comment

AmiDavidW Feb 26, 2019

Choose a reason for hiding this comment

ejona86 Feb 27, 2019

Choose a reason for hiding this comment

ejona86 Feb 27, 2019

Choose a reason for hiding this comment

AmiDavidW Feb 27, 2019

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 Feb 27, 2019

Choose a reason for hiding this comment

AmiDavidW Feb 28, 2019

Choose a reason for hiding this comment

thelinuxfoundation commented Mar 1, 2019

ejona86 commented Mar 6, 2019

carl-mastrangelo left a comment

Choose a reason for hiding this comment

carl-mastrangelo commented Mar 6, 2019

sanjaypujare commented Mar 6, 2019

carl-mastrangelo commented Mar 6, 2019

sanjaypujare commented Mar 6, 2019

carl-mastrangelo commented Mar 6, 2019

carl-mastrangelo commented Mar 6, 2019

AmiDavidW commented Mar 7, 2019