Add GZIP compression defaulting to on#2911
Conversation
Sushisource
force-pushed
the
gzip-compression
branch
from
65d1e41 to
c05271e
Compare
|
|
Sushisource
force-pushed
the
gzip-compression
branch
from
559914a to
c05271e
Compare
| import javax.annotation.Nullable; | ||
|
|
||
| /** Selects transport-level gRPC compression for service calls. */ | ||
| public enum GrpcCompression { |
There was a problem hiding this comment.
Using an enum locks us out of supporting custom compressors if gRPC ever decides to stabilize it. Instead, make it a regular class with private constructor and a single static member GZIP (no compression should be represented by null). This is functionally equivalent to current design, and we still don't expose custom compressors at this point, but it will make future changes easier.
There was a problem hiding this comment.
I don't think we're ever going to need to support a custom compressor. After all, the server has to support it. Maybe we'll add support for zstd or snappy or something, but they can easily be added as variants to this enum (with options).
| builder.useTransportSecurity(); | ||
| } | ||
|
|
||
| builder.decompressorRegistry(options.getGrpcCompression().getDecompressorRegistry()); |
There was a problem hiding this comment.
Compression, decompression and advertising should be 3 independent settings. There are valid scenarios to support decompression without enabling compression, or to support decompression without advertising it (for compatibility with servers that ignore Message-Accept-Encoding). Disabling compression should not impact the others.
Decompressor registry APIs are experimental in gRPC right now. I think we should not call them at all, and not expose the settings to control it. I could be convinced to allow disabling advertising as a separate boolean option marked experimental. But I don't think we should allow disabling decompression entirely until the relevant upstream APIs are stabilized.
There was a problem hiding this comment.
Ah, OK fair point about the API being experiemental. My preference then would be for the no-compression option to simply not set the compressor and otherwise not change anything, rather than having more knobs
|
|
||
| return ClientInterceptors.intercept( | ||
| channel, | ||
| new GrpcCompressionInterceptor(options.getGrpcCompression()), |
There was a problem hiding this comment.
Nit: I would say GrpcCompressionInterceptor should only be added to the chain if compression is enabled, but it's not a big deal one way or the other.
| private Collection<ClientInterceptor> grpcClientInterceptors; | ||
| private Scope metricsScope; | ||
| private boolean apiKeyProvided; | ||
| private GrpcCompression grpcCompression = GrpcCompression.GZIP; |
There was a problem hiding this comment.
We should have one release cycle where compression is available but not enabled by default.
There was a problem hiding this comment.
Haven't done that with the other SDKs. This change is I think quite safe, and if we have it off by default we basically won't get any usage. Turning it off I think is sufficient for anyone who happens to have some kind of broken proxy setup that messes things up.
3 participants
What was changed
Add gzip compression, defaulting on with opt-out
Why?
Save on data! Parity with other SDKs
Checklist
Closes
How was this tested:
Added simple unit test that headers are set properly. More comprehensive testing was performed in Core to validate server actually compresses bytes. Existing integ tests are verifying nothing is breaking, since this value defaults on.
Any docs updates needed?