-
Notifications
You must be signed in to change notification settings - Fork 103
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
Manage transport lifecycle from dispatcher #529
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,12 @@ func (i *Inbound) SetRegistry(registry transport.Registry) { | |
i.registry = registry | ||
} | ||
|
||
// Transports returns the inbound's HTTP transport. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we put a TODO here to route the http.Transport through here? |
||
func (i *Inbound) Transports() []transport.Transport { | ||
// TODO factor out transport and return it here. | ||
return []transport.Transport{} | ||
} | ||
|
||
// Start starts the inbound with a given service detail, opening a listening | ||
// socket. | ||
func (i *Inbound) Start() error { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,15 @@ import "context" | |
|
||
// Outbound is the common interface for all outbounds | ||
type Outbound interface { | ||
// Transports returns the transports that used by this outbound, so they | ||
// can be collected for lifecycle management, typically by a Dispatcher. | ||
// | ||
// Though most outbounds only use a single transport, composite outbounds | ||
// may use multiple transport protocols, particularly for shadowing traffic | ||
// across multiple transport protocols during a transport protocol | ||
// migration. | ||
Transports() []Transport | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't composite outbounds compose the transport, such that it would still be represented as a single transport? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is for the case like this:
The remotely configured outbound would need to return the grpc and tchannel transports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @breerly composite transports would defeat the deduplicator. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aaah, thanks for the clarification. |
||
|
||
// Sets up the outbound to start making calls. | ||
// | ||
// This MUST block until the outbound is ready to start sending requests. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,12 @@ func (i *Inbound) Channel() Channel { | |
return i.ch | ||
} | ||
|
||
// Transports returns the underlying Transport for this Inbound. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo |
||
func (i *Inbound) Transports() []transport.Transport { | ||
// TODO factor out transport and return it here. | ||
return []transport.Transport{} | ||
} | ||
|
||
// Start starts the TChannel inbound transport. This immediately opens a listen | ||
// socket. | ||
func (i *Inbound) Start() error { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,12 @@ func (o *Outbound) WithTracer(tracer opentracing.Tracer) *Outbound { | |
return o | ||
} | ||
|
||
// Transports returns the underlying TChannel Transport for this outbound. | ||
func (o *Outbound) Transports() []transport.Transport { | ||
// TODO factor out transport and return it here. | ||
return []transport.Transport{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why instantiate here? Shouldn't the transport pass an instance of itself into the outbound? Wouldnt this just return that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is an empty array, until I refactor TChannel to use a Transport. |
||
} | ||
|
||
// Start starts the TChannel outbound. | ||
func (o *Outbound) Start() error { | ||
// TODO: Should we create the connection to HostPort (if specified) here or | ||
|
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.
Suggest a more specific name for this
mu
, perhapsstartedtransportsMutex
.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.
This covers all changes to started* in scope.