fishy commented on code in PR #2973:
URL: https://github.com/apache/thrift/pull/2973#discussion_r1588541414
##########
lib/go/thrift/header_transport.go:
##########
@@ -449,6 +455,11 @@ func (t *THeaderTransport) parseHeaders(ctx
context.Context, frameSize uint32) e
}
t.protocolID = THeaderProtocolID(protoID)
+ // Reset writeTransforms to the ones from cfg, as we are going to add
+ // compression transforms from what we read, we don't want to accumulate
+ // different transforms read from different requests
+ t.writeTransforms = t.cfg.GetTHeaderTransforms()
Review Comment:
a few notes here:
1. in TSimpleServer we use the same THeaderProtocol instance for incoming
and outgoing, while in TStandardClient we use different THeaderProtocol
instances, so this behavior should only affect servers (clients would not use
the THeaderProtocol read for writing)
2. the consideration here (and the NOTE added in `AddTransform`) is for a
hypothetical scenario: if a server have a long client connection, but the long
client connection is actually multiple clients (e.g. it's a proxy), and those
multiple clients use different compression transforms (we currently only
support zlib, but the spec also defined snappy so we might support that some
day in the future), we don't want to add different compression transforms
cumulatively and doing both, but just use the same compression used by the
request in the response. Maybe we should mark `AddTransform` as deprecated
instead? the new feature in TConfiguration should be enough to replace that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
