Skip to content
This repository was archived by the owner on Oct 29, 2024. It is now read-only.

Add pool size parameter to client constructor #534

Merged
merged 8 commits into from
Nov 17, 2017

Conversation

vaniakov
Copy link
Contributor

@vaniakov vaniakov commented Nov 6, 2017

This pull request relates to the issue Connection pool is full, discarding connection. To reproduce it you need to create more than 10 (default size of the urllib3 connection pool) simultaneous connection to the database from the same client object.

@@ -64,6 +66,7 @@ def __init__(self,
port=8086,
username='root',
password='root',
pool_size=10,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to cast this value to an int, like we do for the port parameter in line 81.

Copy link
Contributor Author

@vaniakov vaniakov Nov 16, 2017

Choose a reason for hiding this comment

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

@xginn8 makes sense.

@vaniakov
Copy link
Contributor Author

@xginn8 nice catch!

@xginn8
Copy link
Collaborator

xginn8 commented Nov 17, 2017

thanks for contributing!

@xginn8 xginn8 merged commit 530b4c5 into influxdata:master Nov 17, 2017
@vaniakov vaniakov deleted the contribution branch November 17, 2017 09:10
@hppylbb
Copy link

hppylbb commented Jan 6, 2018

Hi all, sorry but I find this issue is still exist in the master branch, the parameter pool_size is invalid even we added it to __init__ method. I think that

self._session.mount(self._scheme, adapter)
should be modified to "self._session.mount(self._scheme+'://', adapter)",
because after executing this line, the "session.adapters" will have three members: "OrderedDict([('https://', <requests.adapters.HTTPAdapter object at 0x7f0c9c123890>), ('http://', <requests.adapters.HTTPAdapter object at 0x7f0c9404e190>), ('http', <requests.adapters.HTTPAdapter object at 0x7f0c9e874290>)]))", the https:// and http:// adapters are added here https://github.com/requests/requests/blob/3dc84cde5b785a85cb20eb2f0d4530fd568a6af7/requests/sessions.py#L391-L393 while initializing, and when you execute https://github.com/requests/requests/blob/3dc84cde5b785a85cb20eb2f0d4530fd568a6af7/requests/sessions.py#L691, you want to get the adapter 'http' which is mounted at
self._session.mount(self._scheme, adapter)
(the third one in OrderedDict), but here you got a incorrect adapter which is http:// (the second one in OrderedDict), this adapter doesn't contain the 'pool_size' parameter we added to __init__ method while initializing InfluxDBClient.

So while we write_points to influxdb, we still use the default pool_size=10, then we run into issue #349.

If we add '://' to self._session.mount(self._scheme+'://', adapter), then the OrderedDict will only contains two adapters: OrderedDict([('https://', <requests.adapters.HTTPAdapter object at 0x7f0c9c123890>), ('http://', <requests.adapters.HTTPAdapter object at 0x7f0c9404e190>)]), the http:// adapter will be overrided by our session mount method, then we can get the correct http adapter with the pool_size we added.

Sorry but I am a newbee of github, cc @xginn8 @vaniakov for notification this comment.

@vaniakov
Copy link
Contributor Author

@hppylbb you are correct. Working on the new PR.

@vaniakov vaniakov mentioned this pull request Mar 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants