-
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
HTTP transport constructors refactor #540
Conversation
@kriskowal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bombela, @abhinav and @AlexAPB to be potential reviewers. |
f37a7f3
to
170e5ec
Compare
a17af61
to
f3bacc5
Compare
170e5ec
to
fec4831
Compare
fec4831
to
1d8bf3a
Compare
@@ -26,12 +26,15 @@ import ( | |||
"sync" | |||
"time" | |||
|
|||
"github.com/opentracing/opentracing-go" |
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
f3bacc5
to
8c5f949
Compare
1d8bf3a
to
c9aec00
Compare
c9aec00
to
b07d4a7
Compare
b07d4a7
to
1d5b97d
Compare
1d5b97d
to
95f6fb0
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.
I think this might be my favorite PR of all time.
@@ -123,19 +121,17 @@ func runTChannelClient(b *testing.B, c *tchannel.Channel, hostPort string) { | |||
} | |||
|
|||
func Benchmark_HTTP_YARPCToYARPC(b *testing.B) { | |||
httpTransport := yhttp.NewTransport() |
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 is great!
Unary: yhttp.NewOutbound( | ||
single.New(hostport.PeerIdentifier("http://localhost:8999"), httpTransport), | ||
), | ||
Unary: httpTransport.NewSingleOutbound("http://localhost:8999"), |
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.
Holy hell this is so much cleaner.
These changes, based on #529, pivot the HTTP transport away from using New* functions on the "transport/http" package to using http.NewTransport() and transport.New* methods for all construction needs. This includes the introduction of transport.NewSingleOutbound, which restores simplicity to the simple case. In all of these cases, the tracer gets threaded from the transport’s configuration into the creation of all dependent inbounds and outbounds. NewSingleOutbound benefits from being able to thread the transport into the single peer chooser behind the scenes.