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

Support for customize the max connections and max pools #773

Closed
wants to merge 1 commit into from

Conversation

pfreixes
Copy link

Related with the ticket #766

@kyleknap
Copy link
Contributor

Cool. Thanks for the pull request. We will take a look at it.

@kyleknap kyleknap added the feature-request This issue requests a feature. label Jan 28, 2016
@pfreixes
Copy link
Author

pfreixes commented Feb 4, 2016

Hi @kyleknap , just as a remember that this PR is on the air :)

@kyleknap
Copy link
Contributor

kyleknap commented Feb 4, 2016

Sorry for the delay here. While I feel that being able to inject your own http adapter is something that we want to add support for, there is a couple implications that other members of the boto org will need to decide upon because as of now we hardly expose anything (except maybe timeouts) about our http layer:

  1. Do we want to expose custom http layer abstractions through the creation of the client or somewhere else like on the session or through an event? I bring this up because I prefer that we do not want to bloat the client signature. I would prefer, if we want to inject http adapters through the client constructor, to put it in the client config since we already have timeout configuration there.

  2. How does this affect botocore's relation with requests? I know we have had discussions about moving off of requests and possibly create our own http layer as we already patch a bunch of parts of requests and every time we vendor in a new version of requests there is something that we need to change in the botocore code to avoid breaking changes. Taking in this change would not allow us to move off of requests. Furthermore if we do more patching/upgrading of requests, it may have an unknown affects on custom http adapters users may have provided and we would have to be very careful about this.

I think we (members of the boto org) need to make decisions on some of the points that I brought up before we can move forward with this pull request. This may take some time depending on what we decided. Also @pfreixes feel free to chime in on any of the points I brought up.

@pfreixes
Copy link
Author

pfreixes commented Feb 5, 2016

Hi @kyleknap thanks for your response, your words are reasonable and I can understand the need to share this discussion with the other members of boto.

The current implementation with the semi ad-hoc implementation of requests - Im not able to see how the requests module and related dependencies such as urllib3 have changed due my poor experience with Boto project and its ecosystem - and the use of a specific class belonging to requests like the HttpAdapter proposed in this PR doesn't help to keep the way to develop Boto open in the future. In fact this PR couple a bit more the dependencies between Boto and requests/urllib3/...

I didn't know that the current requests dependency was nowadays an open discussion. Regarding the importance of this discussion and the side effects for the development of Botcore, the team should get an idea that what they want to do at short term, otherwise future PR - as this one - will keep stuck until this is clarified.

About the use of client config instead of use the kw strategy, bloating the params as you said, is something that is also discutible but definitely the right decision should be that one is aligned with the current implementation. However have in mind that the http_adapter is not a kind of configuration param in a very low level position comparing with other config fields and very coupled with the user code that is using the Botocore. This might be one of the reasons that keep the http_adapter as a constructor param.

Cheers,

@synhershko
Copy link

Perhaps adding a max_concurrency setting from which max_connections and max_poolsize could be derived is something worth considering?

This is a real blocker for us running boto3 + botocore in production, and I'll be happy to help push this forward.

jamesls added a commit to jamesls/botocore that referenced this pull request Sep 8, 2016
This config value allows you to control the size of the
connection pools used in urllib3.  We were previously using the
hardcoded value of 10 (from requests).

As an aside, the implementation for this was much more complicated
than necessary.  I'm thinking on sending a few follow up PRs that
start to clean up some of this technical debt.

Closes boto#773
Closes boto#766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requests a feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants