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

added MONGO_MAX_POOL_SIZE and MONGO_USE_GREENLETS parameters #15

Closed
wants to merge 4 commits into from

Conversation

@akhenakh
Copy link
Contributor

@akhenakh akhenakh commented Nov 13, 2012

I'm using flask-pymongo with gevent and this parameters are usefull for tuning.

if use_greenlets not in (True, False):
raise TypeError('%s_USE_GREENLETS must be a bool' % config_prefix)

max_pool_size

This comment has been minimized.

@dcrosta

dcrosta Nov 14, 2012
Owner

This line doesn't seem to be doing anything ... typo?

This comment has been minimized.

@akhenakh

akhenakh Nov 14, 2012
Author Contributor

oops fixed !

int(max_pool_size)
except ValueError:
raise TypeError('%s_MAX_POOL_SIZE must be an integer' % config_prefix)
kwargs['max_pool_size'] = max_pool_size

This comment has been minimized.

@dcrosta

dcrosta Nov 14, 2012
Owner

Please move this to where we do the other config validation, near line 170 or so.

This comment has been minimized.

@akhenakh

akhenakh Nov 14, 2012
Author Contributor

I'm not sure it is a good idea to be moved around 170 (the else clause of the uri connection) cause I have to do the same tests for the options, that's why I'm doing it after

@dcrosta
Copy link
Owner

@dcrosta dcrosta commented Nov 14, 2012

Hey @ajdavis, I'm a little unfamiliar with how these arguments are intended to be used -- can you offer any insight as to whether this approach is workable and correct?

@ajdavis
Copy link

@ajdavis ajdavis commented Nov 14, 2012

Hey Dan! Summary: max_pool_size is fine, but leave validation up to PyMongo. Don't expose use_greenlets.

max_pool_size is the horrifically-named option to control how many idle sockets the pool will keep open when they're returned to the pool (via end_request, or when a thread dies). So it's better thought of as "max idle sockets before I start closing any more that are returned to the pool."

It's far, far less frequently useful than anyone thinks. I'd venture to call it useless. People want it to mean "max concurrent connections allowed," but PyMongo has no such option. However, if you want to make max_pool_size configurable this is a fine way to do it.

I strongly recommend not validating it at all, since Connection or ReplicaSetConnection will much more thoroughly validate the parameter. Thus it's redundant and likely buggy to do your own validation here.

use_greenlets: please don't set this. We're removing it from public view very soon. Our idea was that you could either do gevent.monkey.patch_all(), or use gevent and set use_greenlets=True. We've changed our minds and we can't think of any good reason not to just do patch_all(), so we're going to legislate that and hide use_greenlets as best we can.

let pymongo Connection validate MAX_POOL_SIZE
changed doc accordingly
@akhenakh
Copy link
Contributor Author

@akhenakh akhenakh commented Nov 15, 2012

Thanks @ajdavis and Dan

@@ -83,6 +83,8 @@ directives:
:class:`~pymongo.connection.Connection`). When used
with PyMongo 2.1, this causes a warning to be
issued and has no effect. Default: ``True``.
``MONGO_MAX_POOL_SIZE`` (optional): The maximum size limit for the connection pool.
Default: ``PyMongo default``.

This comment has been minimized.

@dcrosta

dcrosta Nov 15, 2012
Owner

Per @ajdavis we should probably note that this is the maximum number of idle connections that will be left open.

@dcrosta
Copy link
Owner

@dcrosta dcrosta commented Nov 15, 2012

Looks great other than my one last nitpick comment.

@ajdavis
Copy link

@ajdavis ajdavis commented Nov 15, 2012

Yeah. "The maximum size limit for the connection pool" is copied from PyMongo's docs, but that's a very unfortunately unclear description. I agree with @dcrosta .

@dcrosta
Copy link
Owner

@dcrosta dcrosta commented Dec 15, 2012

Thanks! Sorry it took me a while to get around to merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.