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

capacity/limit is misdocumented #1624

Closed
2 tasks
ExplodingCabbage opened this issue Feb 12, 2017 · 7 comments
Closed
2 tasks

capacity/limit is misdocumented #1624

ExplodingCabbage opened this issue Feb 12, 2017 · 7 comments
Labels
Milestone

Comments

@ExplodingCabbage
Copy link
Contributor

client_reference.rst:

  • Still documents the old limit parameter, not the new capacity one
  • Lists the default value for all the connector subclasses as None, rather than 20.

I'd PR a fix, but as I noted in #1601 (comment) I disagree with @fafhrd91's change to the limit/capacity functionality so I'll wait for that argument to be resolved first - no sense documenting something that I'm currently arguing you guys should revert!

@fafhrd91
Copy link
Member

i agree with your reasoning, but i still think we need capacity feature.

i suggest rename capacity to max_connections and add max_connections_per_host

@fafhrd91 fafhrd91 added this to the 2.0 milestone Feb 16, 2017
@asvetlov
Copy link
Member

Let don't invite very_long_names_in_java_style.
This is documentation issue.
capacity is good name, if we document limit as obsolete and deprecated the meaning of attribute is obvious. limit is also perfect name but already used.

@ExplodingCabbage regarding to your specific case (web scrapper throttling) -- please use explicit semaphores or something like this. Or, maybe, create a ClientSession instance per site.

aiohttp should have perfect defaults for generic cases.
I believe it means that ClientSession should behave like browser.
Most users don't care about connection limits but they are very sensitive to memory/resource leaks.

@fafhrd91
Copy link
Member

i think we should support per host limit.
only reason is, limit per host it is not that trivial to implement.

@asvetlov
Copy link
Member

good point.
Then let's choose capacity and capacity_per_host as alias for deprecated limit.
They are shorter than max_connections and add max_connections_per_host.
I believe total limit is much more wider used than limit per host.

@fafhrd91
Copy link
Member

but maybe we should just leave limit in place, just change it's meaning.
and add limit_per_host for old limit meaning.

@fafhrd91
Copy link
Member

i renamed capacity back to limit and added limit_per_host

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants