Change client connector's limit semantic

[fafhrd91]

Is limit useful in its current form? It seems confusing. Should it be just total number of connections?

[fafhrd91]

@asvetlov @kxepal @popravich ^^^

[kxepal]

How does it confusing and what's the alternative you want to propose?

[fafhrd91]

I would use as pool_size. Question is does anyone need current semantic of limit?

[kxepal]

pool_size sounds more clear than limit for me assuming the context where it applies.

[fafhrd91]

ok, here is mark limit as deprecated, add new pool_size attribute

[asvetlov]

Agree with limit deprecation but dislike pool_size name.
capacity, maxsize or size looks better for my taste.

[fafhrd91]

i like capacity

[asvetlov]

Cool!

[fafhrd91]

let's use maxsize, seems all aioxxx libraries use maxsize

[asvetlov]

+1 for capacity
+0 for maxsize
-1 for pool_size

[kxepal]

I share @asvetlov votes.

[fafhrd91]

ok, lets use "capacity"

[fafhrd91]

in master

[ExplodingCabbage]

I think this was a bad change.

Is limit useful in its current form?

Yes, to me - I use aiohttp for web scraping in an application that frequently scrapes multiple sites simultaneously. I don't really need a global connection limit, but I do benefit from having a per-site connection limit as a crude way of throttling how much load I put on the sites that I'm scraping (since I don't want to nuke some poor student website with 500 simultaneous requests from my high-bandwidth cloud server).

So the existing functionality isn't outright useless. Also, while I don't know if aiohttp tries to follow SemVer, this is surely a violation - you're planning on removing a feature completely in a minor version release.

If you're determined to kill off the old limit functionality, though, then I'll point out that I also don't think you've improved the parameter name. capacity isn't really any clearer to me than limit was, and there's this comment in the code that provides a clear hint to what a better name would be:

# The capacity defines the maximum number of concurrent connections

A rule of thumb that I always apply is that short comments stating what a variable means are a code smell, since they suggest that you could've just named the variable to whatever you're putting in the comment. In this case, why not rename the variable to max_concurrent_connections? That would make the comment unnecessary.

[fafhrd91]

we can re-introduce limit as max_conns_per_host

[fafhrd91]

will add capacity_per_host before 2.0

[ExplodingCabbage]

If I'm understand correctly, you guys have now released a backwards-incompatible API change in a patch version update?

[ExplodingCabbage]

Aha, no, I see - there release targets the 1.3 branch which doesn't have the change to capacity. Cool, that works. :) 👍

[lock]

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.