[python] fix: Configure python urllib3 connection pool size#17323
Conversation
This was removed in OpenAPITools#16802, but using a higher value than 1, or at least making this configurable makes complete sense. Without this, we get a lot of these log messages: [ WARNING] Connection pool is full, discarding connection:
| """ | ||
| {{/asyncio}} | ||
| {{^asyncio}} | ||
| self.connection_pool_maxsize = multiprocessing.cpu_count() * 5 |
There was a problem hiding this comment.
Can we fix it with a static value as before? (it was set to 4 before #16802)
I've had several issues in the past while running services in containers where the CPU count is most of the time the one from the host but not the one from set on the container itself.
There was a problem hiding this comment.
Before #16802, the code was
if maxsize is None:
if configuration.connection_pool_maxsize is not None:
maxsize = configuration.connection_pool_maxsize
else:
maxsize = 4So it was set to 4 only if configuration.connection_pool_maxsize was explicitly set to None, instead of letting it default to multiprocessing.cpu_count() * 5. I'm reverting configuration.mustache to the same code it had before #16802.
From how I understand this setting in urllib3, you should not get more connections than you have threads. This is only an upper limit.
TBH, this fix is most convenient for our use-case (I think). But it would actually be cleanest for the OpenAPI generator to not set any custom default, go with urllib3's default, and make it configurable. What do you think?
There was a problem hiding this comment.
Ah yes, you are right, I just re-read #16802: so multiprocessing.cpu_count() * 5 was indeed most probably the most default value.
Then:
- Let's roll back to the previous value
multiprocessing.cpu_count() * 5to restore the previous behavior. - It's still changeable once the
Configurationobject has been created, so it can be adjusted on a per-client basis.
There was a problem hiding this comment.
Great, thanks! Then the current state of the PR is good and we can resolve?
wing328
changed the title
4 participants
This was removed in #16802, but using a higher value than 1, or at least making this configurable makes complete sense.
Without this, we get a lot of these log messages:
[ WARNING] Connection pool is full, discarding connection:
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming 7.1.0 minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)@cbornet (2017/09) @tomplus (2018/10) @krjakbrjak (2023/02) @fa0311 (2023/10) @multani (2023/10) @wing328