-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TChannel Constructors Refactor #543
Conversation
@kriskowal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abhinav, @breerly and @AlexAPB to be potential reviewers. |
2943e4c
to
4ba832b
Compare
c2a1e68
to
0eedc06
Compare
4ba832b
to
a4acbac
Compare
0eedc06
to
8288865
Compare
a4acbac
to
19b3270
Compare
8288865
to
2aaac8f
Compare
19b3270
to
dcb7fef
Compare
2aaac8f
to
af0c6cf
Compare
af0c6cf
to
1a312e1
Compare
dcb7fef
to
0dc0017
Compare
1a312e1
to
0ac4b3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of small things. Looks really good
"go.uber.org/yarpc/transport" | ||
) | ||
|
||
type inboundConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used
@@ -1,96 +1,71 @@ | |||
// Copyright (c) 2016 Uber Technologies, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need zee licenses!
package tchannel | ||
|
||
import ( | ||
"context" | ||
"io" | ||
|
||
"github.com/uber/tchannel-go" | ||
"go.uber.org/atomic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import order
if config.name == "" { | ||
err = fmt.Errorf("can't instantiate TChannelChannelTransport without channel or service name option") | ||
} else { | ||
ch, err = tchannel.NewChannel(config.name, &tchannel.ChannelOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is during initialization, should we just panic on errors?
// | ||
// All inbounds must have been assigned a registry to accept inbound requests. | ||
func (t *ChannelTransport) Start() error { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space?
return err | ||
} | ||
|
||
t.addr = t.ch.PeerInfo().HostPort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we extract the t.addr grabbing into a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to consider in a follow-up. This mechanically extracted from the old code.
// channel to make requests to any connected peer. | ||
func (t *ChannelTransport) NewOutbound() *ChannelOutbound { | ||
return &ChannelOutbound{ | ||
started: atomic.NewBool(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to set these every time, we can initialize started
as a atomic.Bool and it will automatically be set to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roger, changing to value instead of pointer, using the zero value.
|
||
serverTChannel := ytchannel.NewChannelTransport( | ||
ytchannel.WithServiceName("server"), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0ac4b3f
to
d5d55ee
Compare
@willhug addressed, but keeping the panics out, just to be safe. |
|
||
tchannelTransport := tchannel.NewChannelTransport( | ||
tchannel.WithListenAddr(":8082"), | ||
tchannel.WithServiceName("yarpc-test"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we can call for them? I don't love how they have to specify "yarpc-test".
Based on #542, this change adds tchannel.NewChannelTransport as the new root object for constructing inbounds and outbounds using a wrapped TChannel, and removes the old Inbound/Outbound types.
The name and types deliberately leave open tchannel.NewTransport as a possibility post-1.0, which would not wrap a Channel and consequently be usable with transport.NewOutbound(chooser).