-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Gzip decompression #476
Gzip decompression #476
Conversation
Similar to what grpc-go does, as otherwise it breaks grpc-web
Thank you for this. It looks like a good approach at first glance. I am going to go over the implementation in more depth in the next couple of days, primarily to see if I can spot details that may cause trouble (as in major breaking changes, for example) with further enhancements or other planned features. A bit later on, @LucioFranco can help us out sorting out the details. |
@vbfox I looked at this in a little more detail and I think it's neat. However, we can't proceed yet because we need to figure out how to best to integrate this with channels and clients. I understand this PR is not concerned with that (and it's fine) but wee need to have a clear path forward. We may want to make some changes to the transport before integrating this. It may take a little bit though. Here are some thoughts I had while going through this. Nothing actionable yet, just things we may want to consider.
We'll update with specific steps forward as soon as possible. Thanks again. |
@vbfox Sorry for the slow progress on this. Is this still something you're interested in working on? If so I would like to help reviewing. If not I also wouldn't mind taking over the implementation. I also asked a general question about how the spec requires you to implement compression here. Maybe you're able to help. |
@davidpdrsn I'm interested in gzip compression being available 😄 I can continue working on it but if someone wants to take over no problem, the remaining issues were more about how the project wants to approach integrating Compression and i'm not implicated enough in tonic design for that I admit :) |
Awesome! I'm gonna help out with reviewing then. I guess the first thing to do is get it up to date with the current Here are my comments about what @alce said.
I agree that going with traits to support custom algorithms probably isn't /// Which encodings are enabled?
///
/// Used to construct the `grpc-accept-encoding` header and determine if the
/// `grpc-encoding` of an incoming messsage is supported.
pub(crate) struct MessageEncodingConfig {
gzip: bool,
// more encodings added here later
}
/// Which encoding should be used for a message
///
/// Corresponds to the `grpc-encoding` header.
pub(crate) MessageEncoding {
Gzip,
// more encodings added here later
} Where This is similar to how compression/decompression works in We could also have methods to enable/disable the compression on a single
I think only supporting gzip out of the box is fine but if supporting more
I'm not quite sure what code is referenced here or what the implications would |
@vbfox Have you have time to look at what I wrote ☝️? Do you have questions for us? |
@davidpdrsn Yes I looked at it but didn't have time yet to change the code. My main question is, could an adapted version of this PR lands by itself (no API change and add support for the other party sending compressed messages) with another PR later adding sending compressed messages and configuration or do you expect both at the same time ? On the |
We're currently thinking about doing a 0.5 release which will include a few breaking changes and grpc-web support. I would really like to have this feature included in that release as its very useful. However since this isn't a breaking change and therefore not something we have to include in 0.5 I would prefer if we aimed to make a single PR that added a compelling compression feature, meaning we would have support for both sending and receiving compressed messages. If we break things into multiple PRs we risk getting into a situation where we want to ship 0.5 but can't because compression is only half done. I'm still up for taking over the implementation if you don't feel you have the bandwidth for it.
Personally I don't think allowing users to plug in their own compression algorithms is an important feature. For example its not something supported by reqwest or warp. I think that is fine as it simplifies the implementation quite a bit. |
I'll try to find time this weekend to advance on it but If I can't, I think i'll let you take over.
👍🏼 |
Motivation
When a tonic server we deployed receive grpc messages that are compressed (as gzip by a Go client) it currently fail as decompression isn't currently supported (#282)
I wanted to support this limited case as simply as possible.
Solution
I took the solution of only implementing this very specific case because:
On the code decisions:
Compressor
contains compression and decompression method and aims to be/become the equivalent of https://godoc.org/google.golang.org/grpc/encoding#Compressor it could be the trait that users could implement to provide additional compression methods.Decompression
hosts the decompression information provided to 'decode' (A single http header for now)Compression
is the same on the other direction but is only used for responses in cases where the request was compressed in all other cases no compression take place. It also avoid a problem currently existing in envoy grpc-web support and mimics what grpc-go doesTests done:
Fixes #282