configure max idle connections to s3#808
configure max idle connections to s3#808chris-smith-zocdoc wants to merge 3 commits intobuchgr:masterfrom
Conversation
| log.Fatalf("Failed to create default transport: %v", err) | ||
| } | ||
|
|
||
| tr.MaxIdleConns = MaxIdleConns |
There was a problem hiding this comment.
The minio default for MaxIdleConnsPerHost is 16, which was causing excessive tls negotiation even with a modest concurrency level (~200 concurrent connections)
https://github.com/minio/minio-go/blob/1e5fd8a9ce3588100e5be814f564fd78436aa5f4/transport.go#L51-L52
There was a problem hiding this comment.
Which values did you find the best?
There was a problem hiding this comment.
In our setup its rare for us to exceed 200 concurrent, as we tend to be very bursty, 0 -> 200 -> 0 within a very small window. I expect this to change as our baseline load increases though.
We're using Pants and there are two settings that define the max concurrency per client remote_cache_rpc_concurrency / remote_store_rpc_concurrency at 128 each. We'll occasionally have more than one client making requests concurrently, but because they're generally very fast they often don't overlap given our current workload.
| } | ||
|
|
||
| secure := !DisableSSL | ||
| tr, err := minio.DefaultTransport(secure) |
There was a problem hiding this comment.
I believe enabling ClientSessionCache
tr.TLSClientConfig.ClientSessionCache = tls.NewLRUClientSessionCache(64)
may further improve this, but am not familiar enough with it to know if there are any drawbacks. Should I add that in this pr too? Should it be under a separate flag?
There was a problem hiding this comment.
This probably needs some profiling before we consider changing the default. A separate PR would be best.
mostynb
left a comment
mostynb
left a comment
There was a problem hiding this comment.
Hi- thanks for the contribution.
| &cli.IntFlag{ | ||
| Name: "s3.max_idle_conns", | ||
| Usage: "The maximum number of idle connections to use when using the S3 proxy backend.", | ||
| DefaultText: "1024", |
There was a problem hiding this comment.
Is this correct? The link to minio code shows 256 (and 16 per host).
There was a problem hiding this comment.
Do you think I should preserve the existing defaults? In practice I think they're not good for bazel-remote, which I why I changed it here.
MaxIdleConnsPerHost: 16 This is the effective limit in place prior to my change, because we're only working with one host, eg s3.us-east-1.amazonaws.com
MaxIdleConns: 256 This would never be used, since we can't exceed MaxIdleConnsPerHost
I can see the argument for compatibility though, and preserving MaxIdleConnsPerHost: 16, but that would also mean adding a second flag for MaxIdleConns: 256 or setting that value to same as MaxIdleConnsPerHost (16)
| log.Fatalf("Failed to create default transport: %v", err) | ||
| } | ||
|
|
||
| tr.MaxIdleConns = MaxIdleConns |
There was a problem hiding this comment.
Which values did you find the best?
| } | ||
|
|
||
| secure := !DisableSSL | ||
| tr, err := minio.DefaultTransport(secure) |
There was a problem hiding this comment.
This probably needs some profiling before we consider changing the default. A separate PR would be best.
Co-authored-by: Mostyn Bramley-Moore <mostyn@antipode.se>
|
I landed this change with the new default for MaxIdleConns at 1024 (up from effectively 16). Sorry it took so long. |
|
Thanks @mostynb ! |
3 participants
Fixes #675
This resulted in a significant cpu reduction in our environment