Skip to content
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

increase default pool size #2

Merged
merged 2 commits into from
Dec 22, 2016
Merged

increase default pool size #2

merged 2 commits into from
Dec 22, 2016

Conversation

motobob
Copy link

@motobob motobob commented Dec 16, 2016

Problem:
50 connections as a default max is extremely small and outdated.

Solution:
~~make default to almost not limited for a single IP interface. ~~
make it bigger.
If consumer want to limit it or have more they still can

@nalundgaard

{env, [{connection_timeout, 300000}, {pool_size, 50}]}
%% make default pool ~ all possible ports for a single IP interface. e.g. Usual 1025-65535
%% define your pool or other size if you want to limit it or have more IP interfaces
{env, [{connection_timeout, 300000}, {pool_size, 64000}]}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 64000 is too aggressive.
what if any other HTTP client is used on the same node?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there are lhttpC, hakney, gun, shotgun running together, consumer application has a different problem ;)
plus it does not differ from having other non-erlang apps running and everybody should be ready to handle error of no more TCP sockets

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please share alternative proposal.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd left it as is and let a consumer application to decide how much connections is needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

motivation for the change is that I already lost count of how many times people faced an issue of unexpected client failures due to extremely small pool where it always came by surprise that there is any limit at all.
One may say "read the source" and there will be no issues but i feel failing with error on tcp open if more friendly and clearly indicates the issue to the consumer.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is this change may brake existing applications (which use default pool size) after the next release which includes this change. Let's do not do any harm.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with raising the default limit, I am not sure 3 orders of magnitude increase is needed to ameliorate the problem, and I wonder if it's not likely to result in other unexpected problems (like OOM due to thousands of in-flight requests). Pool size is a back pressure mechanism, and I suspect that there are clients that rely on this subconsciously for whom this could cause a significant unexpected problem. is there a problem with just raising to, say, 500 or 1000?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let it be 1000 by default but it does not address the problem that some do not know about the existence of default.

@motobob
Copy link
Author

motobob commented Dec 22, 2016

i'll fix travis in #3

@motobob motobob merged commit 39a3624 into master Dec 22, 2016
@motobob motobob deleted the larger_pool branch December 22, 2016 15:39
@motobob
Copy link
Author

motobob commented Dec 22, 2016

@waisbrot please add me to owners of lhttpc to HEX so i can publish it

@motobob
Copy link
Author

motobob commented Dec 22, 2016

thanks!

motobob pushed a commit that referenced this pull request Dec 26, 2016
Incorporate mabrek/alertlogic changes
motobob pushed a commit that referenced this pull request Dec 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants