You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/12/17 01:19:26 UTC
[GitHub] [thrift] fishy opened a new pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
fishy opened a new pull request #2296:
URL: https://github.com/apache/thrift/pull/2296
Client: go
Define TConfiguration following the spec, and also move the following
configurations scattered around different TTransport/TProtocol into it:
- 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
- TRichTransport
Also define TConfigurationSetter interface for easier TConfiguration
propagation between wrapped TTransports/TProtocols.
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#issuecomment-750454156
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#discussion_r548053200
##########
File path: lib/go/thrift/framed_transport.go
##########
@@ -155,7 +189,7 @@ func (p *TFramedTransport) readFrame() error {
return err
}
size := binary.BigEndian.Uint32(buf)
- if size < 0 || size > p.maxLength {
+ if size < 0 || size > uint32(p.cfg.GetMaxFrameSize()) {
Review comment:
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`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#issuecomment-748816453
LGTM from a quick read but I won't be able to test until the new year.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a change in pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#discussion_r547957019
##########
File path: lib/go/thrift/header_protocol.go
##########
@@ -346,6 +329,13 @@ func (p *THeaderProtocol) Skip(ctx context.Context, fieldType TType) error {
return p.protocol.Skip(ctx, fieldType)
}
+// SetTConfiguration implemtns TConfigurationSetter.
Review comment:
... implem**e**nts ... (there is more than one instance of this typo)
##########
File path: lib/go/thrift/framed_transport.go
##########
@@ -155,7 +189,7 @@ func (p *TFramedTransport) readFrame() error {
return err
}
size := binary.BigEndian.Uint32(buf)
- if size < 0 || size > p.maxLength {
+ if size < 0 || size > uint32(p.cfg.GetMaxFrameSize()) {
Review comment:
The code was there before but I really wonder when size will become negative on an uint32?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#discussion_r548192770
##########
File path: lib/go/thrift/header_protocol.go
##########
@@ -346,6 +329,13 @@ func (p *THeaderProtocol) Skip(ctx context.Context, fieldType TType) error {
return p.protocol.Skip(ctx, fieldType)
}
+// SetTConfiguration implemtns TConfigurationSetter.
Review comment:
Thanks. Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#discussion_r544740347
##########
File path: lib/go/thrift/header_protocol.go
##########
@@ -45,65 +47,49 @@ type THeaderProtocol struct {
// so the underlying transport should be a raw socket transports (TSocket or TSSLSocket),
// instead of rich transports like TZlibTransport or TFramedTransport.
func NewTHeaderProtocol(trans TTransport) *THeaderProtocol {
- p, err := newTHeaderProtocolWithProtocolID(trans, THeaderProtocolDefault)
- if err != nil {
- // Since we used THeaderProtocolDefault this should never happen,
- // but put a sanity check here just in case.
- panic(err)
- }
- return p
+ return newTHeaderProtocolConf(trans, nil)
}
-func newTHeaderProtocolWithProtocolID(trans TTransport, protoID THeaderProtocolID) (*THeaderProtocol, error) {
- t, err := NewTHeaderTransportWithProtocolID(trans, protoID)
- if err != nil {
- return nil, err
- }
- p, err := t.protocolID.GetProtocol(t)
- if err != nil {
- return nil, err
- }
+func newTHeaderProtocolConf(trans TTransport, cfg *TConfiguration) *THeaderProtocol {
+ t := NewTHeaderTransportConf(trans, cfg)
+ p, _ := t.cfg.GetTHeaderProtocolID().GetProtocol(t)
+ PropagateTConfiguration(p, cfg)
return &THeaderProtocol{
transport: t,
protocol: p,
- }, nil
+ cfg: cfg,
+ }
}
type tHeaderProtocolFactory struct {
- protoID THeaderProtocolID
+ cfg *TConfiguration
}
func (f tHeaderProtocolFactory) GetProtocol(trans TTransport) TProtocol {
- p, err := newTHeaderProtocolWithProtocolID(trans, f.protoID)
- if err != nil {
- // Currently there's no way for external users to construct a
- // valid factory with invalid protoID, so this should never
- // happen. But put a sanity check here just in case in the
- // future a bug made that possible.
- panic(err)
- }
- return p
+ return newTHeaderProtocolConf(trans, f.cfg)
}
-// NewTHeaderProtocolFactory creates a factory for THeader with default protocol
-// ID.
-//
-// It's a wrapper for NewTHeaderProtocol
+func (f *tHeaderProtocolFactory) SetTConfiguration(cfg *TConfiguration) {
+ f.cfg = cfg
+}
+
+// NewTHeaderProtocolFactory creates a factory for THeader with default
+// TConfiguration.
func NewTHeaderProtocolFactory() TProtocolFactory {
- return tHeaderProtocolFactory{
- protoID: THeaderProtocolDefault,
- }
+ return NewTHeaderProtocolFactoryConf(nil)
}
-// NewTHeaderProtocolFactoryWithProtocolID creates a factory for THeader with
-// given protocol ID.
-func NewTHeaderProtocolFactoryWithProtocolID(protoID THeaderProtocolID) (TProtocolFactory, error) {
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#discussion_r559158877
##########
File path: lib/go/thrift/header_protocol.go
##########
@@ -34,76 +34,65 @@ type THeaderProtocol struct {
// Will be initialized on first read/write.
protocol TProtocol
+
+ cfg *TConfiguration
+}
+
+// Deprecated: Use NewTHeaderProtocolConf instead.
+func NewTHeaderProtocol(trans TTransport) *THeaderProtocol {
+ return newTHeaderProtocolConf(trans, &TConfiguration{
+ noPropagation: true,
+ })
}
-// 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.
Review comment:
```suggestion
// transport with given TConfiguration.
```
##########
File path: lib/go/thrift/framed_transport.go
##########
@@ -155,7 +189,7 @@ func (p *TFramedTransport) readFrame() error {
return err
}
size := binary.BigEndian.Uint32(buf)
- if size < 0 || size > p.maxLength {
+ if size < 0 || size > uint32(p.cfg.GetMaxFrameSize()) {
Review comment:
I very much doubt this is a legitimate possibility, `int32` seems fine to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy merged pull request #2296: THRIFT-5322: Implement TConfiguration in Go library
Posted by GitBox <gi...@apache.org>.
fishy merged pull request #2296:
URL: https://github.com/apache/thrift/pull/2296
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org