THRIFT-5322: Implement TConfiguration in Go library #2296

fishy · 2020-12-17T01:19:10Z

Client: go

Define TConfiguration following the spec, and also move the following
configurations scattered around different TTransport/TProtocol into it:

connect and socket timeouts for TSocket and TSSLSocket
tls config for TSSLSocket
max frame size for TFramedTransport
strict read and strict write for TBinaryProtocol
proto id for THeaderTransport

Also add TConfiguration support for the following and their factories:

THeaderTransport and THeaderProtocol
TBinaryProtocol
TCompactProtocol
TFramedTransport
TSocket
TSSLSocket

Also define TConfigurationSetter interface for easier TConfiguration
propagation between wrapped TTransports/TProtocols , and add
implementations to the following for propagation
(they don't use anything from TConfiguration themselves):

StreamTransport
TBufferedTransport
TDebugProtocol
TJSONProtocol
TSimpleJSONProtocol
TZlibTransport

TConfigurationSetter are not implemented by the factories of the
"propagation only" TTransports/TProtocols, if they have a factory. For
those use cases, TTransportFactoryConf and TProtocolFactoryConf are
provided to wrap a factory with the ability to propagate TConfiguration.

Also add simple sanity check for TBinaryProtocol and TCompactProtocol's
ReadString and ReadBinary functions. Currently it only report error if
the header length is larger than MaxMessageSize configured in
TConfiguration, for simplicity.

fishy · 2020-12-17T01:20:22Z

lib/go/thrift/header_protocol.go


-// NewTHeaderProtocolFactoryWithProtocolID creates a factory for THeader with
-// given protocol ID.
-func NewTHeaderProtocolFactoryWithProtocolID(protoID THeaderProtocolID) (TProtocolFactory, error) {


note that this is a breaking change from a previous version on master branch. however it is not a breaking change from 0.13.0.

dcelasun · 2020-12-21T07:33:34Z

LGTM from a quick read but I won't be able to test until the new year.

Jens-G

Good work!

LGTM overall. I only did a code review, did not run any tests on it. Only minor/cosmetic issues found.

Jens-G · 2020-12-23T13:21:56Z

lib/go/thrift/framed_transport.go

 	}
 	size := binary.BigEndian.Uint32(buf)
-	if size < 0 || size > p.maxLength {
+	if size < 0 || size > uint32(p.cfg.GetMaxFrameSize()) {


The code was there before but I really wonder when size will become negative on an uint32?

Before p.maxLength actually has the type of uint32 instead of int32.

To become negative when converting to int32 the uint32 value needs to be at least 2GB (MaxInt32 is 1<<31-1). I'm not sure how much do we really care about that use case. If we do care about it I can change the types of MaxMessageSize and MaxFrameSize both from int32 to uint32.

I very much doubt this is a legitimate possibility, int32 seems fine to me.

Jens-G · 2020-12-23T13:24:42Z

lib/go/thrift/header_protocol.go

 	return p.protocol.Skip(ctx, fieldType)
 }

+// SetTConfiguration implemtns TConfigurationSetter.


... implements ... (there is more than one instance of this typo)

Thanks. Fixed.

fishy · 2020-12-23T20:07:48Z

I moved timeouts from TSocket/TSSLSocket to TConfiguration, and added propagation support to more TTransport/TProtocol implementations, and also updated the commit message/PR description. I believe now TMemoryBuffer is the only one not supporting either TConfiguration or TConfigurationSetter, as it doesn't need any configurations itself and also it's always the terminal TTransport that it has nothing underneath to propagate into.

Please take another look.

dcelasun

Finally had a chance to play around with this. I've tested framed transport and binary protocol, works as expected.

Thanks for working on this :)

dcelasun · 2021-01-17T11:04:26Z

lib/go/thrift/framed_transport.go

 	}
 	size := binary.BigEndian.Uint32(buf)
-	if size < 0 || size > p.maxLength {
+	if size < 0 || size > uint32(p.cfg.GetMaxFrameSize()) {


I very much doubt this is a legitimate possibility, int32 seems fine to me.

dcelasun · 2021-01-17T11:04:42Z

lib/go/thrift/header_protocol.go

-// NewTHeaderProtocol creates a new THeaderProtocol from the underlying
-// transport with default protocol ID.
+// NewTHeaderProtocolConf creates a new THeaderProtocol from the underlying
+// transport with gien TConfiguration.


Suggested change

// transport with gien TConfiguration.

// transport with given TConfiguration.

Client: go Define TConfiguration following the spec, and also move the following configurations scattered around different TTransport/TProtocol into it: - connect and socket timeouts for TSocket and TSSLSocket - tls config for TSSLSocket - max frame size for TFramedTransport - strict read and strict write for TBinaryProtocol - proto id for THeaderTransport Also add TConfiguration support for the following and their factories: - THeaderTransport and THeaderProtocol - TBinaryProtocol - TCompactProtocol - TFramedTransport - TSocket - TSSLSocket Also define TConfigurationSetter interface for easier TConfiguration propagation between wrapped TTransports/TProtocols , and add implementations to the following for propagation (they don't use anything from TConfiguration themselves): - StreamTransport - TBufferedTransport - TDebugProtocol - TJSONProtocol - TSimpleJSONProtocol - TZlibTransport TConfigurationSetter are not implemented by the factories of the "propagation only" TTransports/TProtocols, if they have a factory. For those use cases, TTransportFactoryConf and TProtocolFactoryConf are provided to wrap a factory with the ability to propagate TConfiguration. Also add simple sanity check for TBinaryProtocol and TCompactProtocol's ReadString and ReadBinary functions. Currently it only report error if the header length is larger than MaxMessageSize configured in TConfiguration, for simplicity.

Client: go This fixes a bug introduced in #2296, that we mixed the preferred proto id and the detected proto id, which was a bad idea. This change separates them, so when we propagate TConfiguration, we only change the preferred one, which will only be used for new connections, and leave the detected one from existing connections untouched.

Client: go This fixes a bug introduced in apache#2296, that we mixed the preferred proto id and the detected proto id, which was a bad idea. This change separates them, so when we propagate TConfiguration, we only change the preferred one, which will only be used for new connections, and leave the detected one from existing connections untouched.

Client: go This fixes a bug introduced in apache#2296, that we mixed the preferred proto id and the detected proto id, which was a bad idea. This change separates them, so when we propagate TConfiguration, we only change the preferred one, which will only be used for new connections, and leave the detected one from existing connections untouched. Also add a test for it.

Client: go This fixes a bug introduced in #2296, that we mixed the preferred proto id and the detected proto id, which was a bad idea. This change separates them, so when we propagate TConfiguration, we only change the preferred one, which will only be used for new connections, and leave the detected one from existing connections untouched. Also add a test for it.

fishy requested review from Jens-G and dcelasun December 17, 2020 01:19

fishy commented Dec 17, 2020

View reviewed changes

fishy force-pushed the go-tconfiguration branch from c047f4e to 173ea94 Compare December 17, 2020 01:23

Jens-G reviewed Dec 23, 2020

View reviewed changes

fishy force-pushed the go-tconfiguration branch from 173ea94 to 1dbf3ac Compare December 23, 2020 20:04

fishy force-pushed the go-tconfiguration branch 4 times, most recently from 2e93211 to 36ae85f Compare December 28, 2020 18:23

dcelasun approved these changes Jan 17, 2021

View reviewed changes

fishy force-pushed the go-tconfiguration branch 2 times, most recently from baad75e to af0664c Compare January 17, 2021 19:07

fishy force-pushed the go-tconfiguration branch from af0664c to fd53d7a Compare January 17, 2021 20:24

fishy merged commit c4d1c0d into apache:master Jan 17, 2021

fishy deleted the go-tconfiguration branch January 17, 2021 20:24

fishy mentioned this pull request Jan 22, 2021

THRIFT-5322: THeaderTransport protocol id fix #2316

Merged

leitzler mentioned this pull request Oct 28, 2024

THRIFT-5828: reduce over-allocation in Go binary protocol #3057

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-5322: Implement TConfiguration in Go library #2296

THRIFT-5322: Implement TConfiguration in Go library #2296

Uh oh!

fishy commented Dec 17, 2020 •

edited

Loading

Uh oh!

fishy Dec 17, 2020

Uh oh!

dcelasun commented Dec 21, 2020

Uh oh!

Jens-G left a comment

Uh oh!

Jens-G Dec 23, 2020

Uh oh!

fishy Dec 23, 2020

Uh oh!

dcelasun Jan 17, 2021

Uh oh!

Jens-G Dec 23, 2020

Uh oh!

fishy Dec 23, 2020

Uh oh!

fishy commented Dec 23, 2020

Uh oh!

dcelasun left a comment

Uh oh!

dcelasun Jan 17, 2021

Uh oh!

dcelasun Jan 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// transport with gien TConfiguration.
	// transport with given TConfiguration.

THRIFT-5322: Implement TConfiguration in Go library #2296

THRIFT-5322: Implement TConfiguration in Go library #2296

Uh oh!

Conversation

fishy commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcelasun commented Dec 21, 2020

Uh oh!

Jens-G left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fishy commented Dec 23, 2020

Uh oh!

dcelasun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fishy commented Dec 17, 2020 •

edited

Loading