Add HTTP transport with settings to support multiple connections #2685
Conversation
super-harsh
requested review from
davefellows,
theunrepentantgeek,
matthchr and
babbageclunk
as code owners
| ForceAttemptHTTP2: false, | ||
| MaxIdleConns: 100, | ||
| MaxIdleConnsPerHost: 10, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ExpectContinueTimeout: 1 * time.Second, |
There was a problem hiding this comment.
Suggest adding an end-of-line comment to each of these values explaining why these values were chosen.
| }).DialContext, | ||
| ForceAttemptHTTP2: false, // default is true; since HTTP/2 multiplexes a single TCP connection. we'd want to use HTTP/1, which would use multiple TCP connections. | ||
| MaxIdleConns: 100, // default transport value | ||
| MaxIdleConnsPerHost: 10, // default is 2, so we want to increase the number to use establish more connections. |
There was a problem hiding this comment.
This is missing TLS min version, which the SDK has I think?
defaultTransport := &http.Transport{
Proxy: http.ProxyFromEnvironment,
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
}).DialContext,
ForceAttemptHTTP2: true,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
TLSClientConfig: &tls.Config{
MinVersion: tls.VersionTLS12,
},
}
| // to establish multiple TCP ARMClient connections to avoid throttling. | ||
| // TODO: Use https://github.com/Azure/go-armbalancer here once its prod ready. | ||
| httpTransport := &http.Transport{ | ||
| Proxy: http.ProxyFromEnvironment, |
There was a problem hiding this comment.
Ahh one more thing -- we should have a single global one of these, not 1 per call to this function (that's how the SDK does it as well, see the transport_default_http_client link I gave below.
There was a problem hiding this comment.
So rather than having a httpClient per GenericClient, we want to share a default httpClient across all GenericClients?
There was a problem hiding this comment.
Share the transport, the HTTPClient can be constructed 1 for each.
There was a problem hiding this comment.
The reason for that is because the transport == connection pool == expensive to create. Golang has a single default transport and so does the Azure SDK, so we want to mirror that experience otherwise it would be easy to accidentally end up creating a lot more connections than we expected (because we constructed different GenericClient's)
Closes #2672
Special notes for your reviewer:
Did some manual testing around it to make 2500 successful calls using a single controller and multiple controllers. Before the change, we were running into
SubscriptionRequestsThrottlederror.As confirmed by @matthchr, we can use go-armbalancer project to tackle this problem better by mid feb.
How does this PR make you feel:
If applicable: