-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Description
Use case(s) - what problem will this feature solve?
I've been experimenting with enabling compression, after discovering that it wasn't enabled by default.
While building up some automated tests, I was using the health package for my testing.
Notably, the response format for the Check RPC is extremely small
https://github.com/grpc/grpc-proto/blob/23f5b568eefcb876e6ebc3b01725f1f20cff999e/grpc/health/v1/health.proto#L33-L41
As it stands, a Server can opt-in to compression by calling grpc.SetSendCompressor in an interceptor or a handler, or compression will be reflected against whatever encoding the client used.
However, there is no way that I can see to make the compression consider the size of the encoded data before making a compression decision. Although the Compressor is an interface that could decide not to modify the body, the grpc-encoding header is always set to the name of the compressor.
This is only likely to make a difference when there are a large number of small or high-entropy responses, but that's probably a fairly common scenario across RPCs found in the wild?
Proposed Solution
Perhaps a way for the Compressor interface to signal that it's opting out? A simple backwards-compatible change should be to allow Compress(w io.Writer) (io.WriteCloser, error) to return a nil for the WriteCloser.
From what I can see from the compress and recvAndDecompress functions, there appears to be an additional hdr that has a bit indicating whether the payload was actually compressed or not - regardless of what the grpc-encoding header says. If the payload isn't being encoded it seems like it should avoid setting the header, but it looks like it would work fine without removing the header.
Alternatives Considered
If the Server RPC implementation knows its payload does not compress well then it could explicitly call grpc.SetSendCompressor("identity"), but this seems like it would be annoying to have to scatter everywhere.
Additional Context
Not that I could think of