Adds snappy support for http_listener_v2#8966
Conversation
srebhan
left a comment
There was a problem hiding this comment.
I would prefer to match the pattern of gzip and construct a reader. Is there any reason to not do it this way?
|
Thanks for the review, I'll take a look at matching the gzip pattern |
|
So after just having another look at the docs, we want to use Decode rather than a NewReader as we need the block format rather than streaming format (specifically for the prometheus remote write use case). Bit more context here |
| } | ||
|
|
||
| // Handle snappy request bodies | ||
| if req.Header.Get("Content-Encoding") == "snappy" { |
There was a problem hiding this comment.
About 20 lines above there's a similar block for gzip. It looks like we should change the "if content-encoding == gzip" to a switch statement that also handles snappy, then move this new block into the switch statement. The snappy decoding needs to happen at the same place gzip does, namely before the MaxBytesReader.
There was a problem hiding this comment.
The NewReader function of snappy would return a reader such as the gzip implementation so the switch would work but the block implementation of snappy that we need (for prometheus remote write) is a Decode which takes in bytes and returns bytes. So I think it needs to be after the reader on 266 unless I'm missing something.
Adds snappy support plus test
Relates to #8682, #8967 - one use is for with prometheus remote write parsing