Limit concurrent S3 connections per host #186
Conversation
This commit limits the number of concurrent connections to encourage connection reuse. `MaxIdleConnsPerHost` was already set to workaround golang/go#13801 ("issue which made Go consume all ephemeral ports"). We observed a similar issue when starting up Loki (using weaveworks `common/aws` throught Cortex) which resulted in thousands of very small S3 requests until memcached was seeded. This failed because of ephemeral port exhaustion: while the application in fact would have reused connections, it already failed as it sent all of them concurrently until most of the requests failed. `MaxConnsPerHost` can be used to limit the overall number of parallel connections.
|
This code is used for more than S3. Given that it is a highly-reused library I would prefer to see parallelism limited at source, rather than deep in the SDK code. Worst case the limit should be configurable, off by default, and commented. |
|
As already mentioned, I'm not really sure this is an all-valid default. Making it configurable would make a bunch of use cases much easier, though. Sadly, modifying the HTTP client's from outside the function seems not really possible (or I was just lost in a hell of type assertions). Optionally passing an HTTP client or transport might be somewhat feasible though. As already discussed in the Loki issue, Loki/Cortex already implemented an improved schema some months ago (which was the new recommended one just few days after we digged into the issue). So on the client-side, there definitely is some improvement works. When reading through this comment again I have the feeling one of those two occurrences of // Use a custom http.Client with the golang defaults but also specifying
// MaxIdleConnsPerHost because of a bug in golang https://github.com/golang/go/issues/13801
// where MaxIdleConnsPerHost does not work as expected. |
Assuming you mean "one of those two occurrences in the comment", I don't think so. The struct is copy-pasted from the Go |
|
@JensErat are you planning to come back to this? |
This commit limits the number of concurrent connections to encourage
connection reuse.
MaxIdleConnsPerHostwas already set to workaroundgolang/go#13801 ("issue which made Go consume
all ephemeral ports").
We observed a similar issue when starting up Loki (using weaveworks
common/awsthrought Cortex) which resulted in thousands of very smallS3 requests until memcached was seeded. This failed because of ephemeral
port exhaustion: while the application in fact would have reused
connections, it already failed as it sent all of them concurrently until
most of the requests failed.
MaxConnsPerHostcan be used to limit the overall number of parallelconnections.
An alternative solution would be to queue requests in the application, but the golang HTTP client already has a queue. Whether a maximum of 100 parallel requests is reasonable or too small might be subject of discussion. I'm not sure if there is a relevant use case of more than 100 parallel S3 streams per instance.
In my experiments, a connection limit larger than
MaxIdleConnsresulted in few connections piling up, but in a rate the system could handle well (with other words: they went through theirTIME_WAITwindow and were removed from the connection list without the system getting close to the ephemeral connection limit).