search

uber / tchannel-go

Go implementation of a multiplexing and framing protocol for RPC calls
http://uber.github.io/tchannel/
MIT License
483 stars 81 forks source link

Update Thrift to v0.13.0 #781

Closed nilebox closed 4 years ago

nilebox commented 4 years ago

tchannel-go is using an outdated Thrift dependency, which causes dependency conflict issues in the projects depending on it.

Examples:

The OpenTelemetry Collector repo includes Jaeger and Prometheus receivers, so it has Go Module dependencies on both - https://github.com/jaegertracing/jaeger and https://github.com/prometheus/prometheus.

When I update the Prometheus dependency to the latest v2.19.0 release, it brings the Thrift v0.13.0 dependency along, which breaks the Jaeger receiver compilation and complains about tchannel-go method mismatch, since it's using older Thrift version.

Seems like there were breaking changes in Thrift API between v0.9.3 and v0.13.0, and the only way to fix this dependency hell is to update uber/tchannel-go to use apache/thrift v0.13.0, or remove a dependency on tchannel-go from Jaeger.

nilebox commented 4 years ago

Note that tchannel-go uses a custom thrift-gen binary attached to the tag https://github.com/uber/tchannel-go/releases/tag/thrift-v1.0.0-dev, e.g. https://github.com/uber/tchannel-go/releases/download/thrift-v1.0.0-dev/thrift-1-linux-x86_64.tar.gz for Linux platform.

This is hardcoded in https://github.com/uber/tchannel-go/blob/1bd5f675d6bcf76fa23d84171acb1bb79999ebd6/scripts/install-thrift.sh#L12

It needs to be updated to v0.13.0 as well, and regenerate the types, otherwise the compilation fails:

# github.com/uber/tchannel-go/thrift/gen-go/meta
thrift/gen-go/meta/meta.go:79:20: not enough arguments in call to oprot.Flush
    have ()
    want (context.Context)
thrift/gen-go/meta/meta.go:103:15: assignment mismatch: 2 variables but error2.Read returns 1 values
thrift/gen-go/meta/meta.go:152:20: not enough arguments in call to oprot.Flush
    have ()
    want (context.Context)
thrift/gen-go/meta/meta.go:176:15: assignment mismatch: 2 variables but error4.Read returns 1 values
thrift/gen-go/meta/meta.go:225:20: not enough arguments in call to oprot.Flush
    have ()
    want (context.Context)
thrift/gen-go/meta/meta.go:249:15: assignment mismatch: 2 variables but error6.Read returns 1 values
thrift/gen-go/meta/meta.go:295:31: cannot use &metaProcessorHealth literal (type *metaProcessorHealth) as type thrift.TProcessorFunction in assignment:
    *metaProcessorHealth does not implement thrift.TProcessorFunction (wrong type for Process method)
        have Process(int32, thrift.TProtocol, thrift.TProtocol) (bool, thrift.TException)
        want Process(context.Context, int32, thrift.TProtocol, thrift.TProtocol) (bool, thrift.TException)
thrift/gen-go/meta/meta.go:296:34: cannot use &metaProcessorThriftIDL literal (type *metaProcessorThriftIDL) as type thrift.TProcessorFunction in assignment:
    *metaProcessorThriftIDL does not implement thrift.TProcessorFunction (wrong type for Process method)
        have Process(int32, thrift.TProtocol, thrift.TProtocol) (bool, thrift.TException)
        want Process(context.Context, int32, thrift.TProtocol, thrift.TProtocol) (bool, thrift.TException)
thrift/gen-go/meta/meta.go:297:36: cannot use &metaProcessorVersionInfo literal (type *metaProcessorVersionInfo) as type thrift.TProcessorFunction in assignment:
    *metaProcessorVersionInfo does not implement thrift.TProcessorFunction (wrong type for Process method)
        have Process(int32, thrift.TProtocol, thrift.TProtocol) (bool, thrift.TException)
        want Process(context.Context, int32, thrift.TProtocol, thrift.TProtocol) (bool, thrift.TException)
thrift/gen-go/meta/meta.go:307:27: not enough arguments in call to processor.Process
    have (int32, thrift.TProtocol, thrift.TProtocol)
    want (context.Context, int32, thrift.TProtocol, thrift.TProtocol)
thrift/gen-go/meta/meta.go:307:27: too many errors
Command exited with non-zero status 2
prashantv commented 4 years ago

Updating to Thrift > 0.11 has a breaking API change (added context.Context to the Thrift interfaces), and updating our library to use the newer version would similarly be a breaking API change for all the clients using TChannel.

We follow semver, and cannot cut a 1.x release that updates to the latest Thrift APIs, which is why we are pinned to to <= 0.11. Cutting a 2.x release is not something we are planning on.

@ys Can you use YARPC's TChannel + Thrift integration in Jaeger, which should remove the dependency on Apache Thrift? This will avoid any dependency constraints required for tchannel-go/thrift from affecting clients using OpenTelemetry.

nilebox commented 4 years ago

@prashantv the issue blocking OpenTelemetry collector has been fixed on the Jaeger side in https://github.com/jaegertracing/jaeger/pull/2311. Feel free to close this issue as "won't fix" if you don't plan to update Thrift dependency in tchannel-go.

prashantv commented 4 years ago

Thanks for the update. Since we won't be able to update without a 2.x release, and that will require significant work for clients, closing this as Wont Fix.