Fix gzip support in socket_listener with tcp sockets #7446
Conversation
internal/content_coding.go
Outdated
| "bytes" | ||
| "compress/gzip" | ||
| "errors" | ||
| "io" | ||
| ) | ||
|
|
||
| // NewStreamContentEncoder returns a writer that will encode the stream | ||
| // according to the encoding type. | ||
| func NewStreamContentEncoder(encoding string, w io.Writer) (io.Writer, error) { |
There was a problem hiding this comment.
This isn't used outside of tests. Are you planning to use it later?
There was a problem hiding this comment.
I'll remove it for now, originally was planning to use it in the socket_writer but ended up leaving it with the batching version. Potentially we could use something like this to reduce memory usage.
internal/content_coding.go
Outdated
| } | ||
|
|
||
| func (w *GzipWriter) Write(p []byte) (int, error) { | ||
| w.z.Reset(w.w) |
There was a problem hiding this comment.
I'm not sure why we start a new gzip stream for each write. Doesn't this prevent the gzip algorithm from getting enough state to compress well?
There was a problem hiding this comment.
It would if the write is small, my idea here was that it would be nice to be able to read each batch. I'm going to remove the Writer completely for now since it isn't used and we can sort this out later.
| ssl.Log.Error("Read error: %v", err) | ||
| } | ||
|
|
||
| scnr := bufio.NewScanner(decoder) |
There was a problem hiding this comment.
We always wrap with decoder here. Wrapping makes sense to me for gzip, but not for identity. If identity, why not pass in c and not wrap at all?
There was a problem hiding this comment.
In the 'identity' case, internal.NewStreamContentDecoder is just returning the reader unwrapped. I thought it would be a bit nicer to push the switch logic into this factory function.
|
I removed the GzipWriter related code, didn't end up using it for now so we can revive and complete it when we have the need. |
|
@danielnelson - I tested this PR and I am seeing issues. I see random TCP disconnects when sending data. Update: ignore, I had some code that was adding a newline at the end of the compressed payload. It was based on the old tests and forgot to remove it. It's all working as expected. Thank you! |
closes #7377
Required for all PRs: