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

Add max_pool_size parameter #2699

Merged
merged 3 commits into from Nov 17, 2020
Merged

Add max_pool_size parameter #2699

merged 3 commits into from Nov 17, 2020

Conversation

Skazza94
Copy link
Contributor

@Skazza94 Skazza94 commented Nov 12, 2020

All the connection pools (UNIX socket/NPipe and SSH) are limited to a pool of 10 threads. This value is hardcoded in the Python code, so it can't be changed.

This pull request adds a max_pool_size parameter in the Client that makes able to tweak the number of thread for each connection pool used by the library. Of couse, by default this value is set to 10.

This feature was requested in #2477. We already opened a PR 1 year ago but we had some problems with the commits signatures.

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
@tcaiazzi
Copy link

I've closed my old PR, hoping that this time it is ok!

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
Copy link
Collaborator

@aiordache aiordache left a comment

Choose a reason for hiding this comment

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

SGTM. Thank you!

@aiordache aiordache added this to the 4.4.0 milestone Nov 16, 2020
Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

One minor issue then LGTM

Comment on lines 106 to 107
max_pool_size=DEFAULT_MAX_POOL_SIZE, credstore_env=None,
use_ssh_client=False):
Copy link
Member

Choose a reason for hiding this comment

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

I would move the newest parameter to the end of the list so that if users are using this constructer with positional parameters, it will continue to work as expected.

Suggested change
max_pool_size=DEFAULT_MAX_POOL_SIZE, credstore_env=None,
use_ssh_client=False):
credstore_env=None, use_ssh_client=False,
max_pool_size=DEFAULT_MAX_POOL_SIZE):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris-crone, I did it. Thanks for the suggestion! 😄

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
@aiordache aiordache merged commit bb1c528 into docker:master Nov 17, 2020
aiordache pushed a commit that referenced this pull request Feb 15, 2021
* Add max_pool_size parameter

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>

* Add client version to tests

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>

* Fix parameter position

Signed-off-by: Mariano Scazzariello <marianoscazzariello@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants