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

handle more connection parameters #20

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ultrabug

ultrabug commented Jan 21, 2013

Hi Dan,

Would you please consider those commits ? They add connection parameters support for :

  • all newer pymongo ReadPreference flags as per [1]
  • use_greenlets flag for the gevent users
  • socket and connect timeouts (ms)

[1] http://api.mongodb.org/python/current/api/pymongo/index.html#pymongo.read_preferences.ReadPreference

Thank you :)

@@ -142,10 +149,13 @@ def key(suffix):
app.config[key('DBNAME')] = parsed['database']
app.config[key('READ_PREFERENCE')] = parsed['options'].get('read_preference')
app.config[key('AUTO_START_REQUEST')] = parsed['options'].get('auto_start_request', True)
app.config[key('USE_GREENLETS')] = parsed['options'].get('use_greenlets', False)

This comment has been minimized.

@dcrosta

dcrosta Jan 22, 2013

Owner

As discussed in #15, we (the PyMongo maintainers and I) prefer not to expose internal details like use_greenlets.

@dcrosta

dcrosta Jan 22, 2013

Owner

As discussed in #15, we (the PyMongo maintainers and I) prefer not to expose internal details like use_greenlets.

This comment has been minimized.

@behackett

behackett Jan 22, 2013

Heh, in PyMongo 2.4.2 we are starting to expose this. Not because it's useful to application developers though. It solves a problem with MasterSlaveConnection. In PyMongo 3.0 (whenever that happens) MasterSlaveConnection will go away, as well as this property.

@behackett

behackett Jan 22, 2013

Heh, in PyMongo 2.4.2 we are starting to expose this. Not because it's useful to application developers though. It solves a problem with MasterSlaveConnection. In PyMongo 3.0 (whenever that happens) MasterSlaveConnection will go away, as well as this property.

This comment has been minimized.

@ajdavis

ajdavis Jan 22, 2013

I believe a Flask app would never want to set use_greenlets True, since it's either running multithreaded (in which case setting use_greenlets would cause bugs) or under Gevent (which would call patch_thread and make use_greenlets unnecessary).

@ajdavis

ajdavis Jan 22, 2013

I believe a Flask app would never want to set use_greenlets True, since it's either running multithreaded (in which case setting use_greenlets would cause bugs) or under Gevent (which would call patch_thread and make use_greenlets unnecessary).

This comment has been minimized.

@ultrabug

ultrabug Jan 24, 2013

You're right indeed, this was something I noted back then when using gevent with threads. This can be ignored, thanks for the clarification guys.

@ultrabug

ultrabug Jan 24, 2013

You're right indeed, this was something I noted back then when using gevent with threads. This can be ignored, thanks for the clarification guys.

app.config[key('USERNAME')] = parsed['username']
app.config[key('PASSWORD')] = parsed['password']
app.config[key('REPLICA_SET')] = parsed['options'].get('replica_set')
app.config[key('MAX_POOL_SIZE')] = parsed['options'].get('max_pool_size')
app.config[key('SOCKET_TIMEOUT_MS')] = parsed['options'].get('socket_timeout_ms', None)
app.config[key('CONNECT_TIMEOUT_MS')] = parsed['options'].get('connect_timeout_ms', None)

This comment has been minimized.

@dcrosta

dcrosta Jan 22, 2013

Owner

I suspect these probably fall under the same "don't expose internal details" criteria as with use_greenlets -- do you have a convincing use case for making these exposed in Flask-PyMongo?

@dcrosta

dcrosta Jan 22, 2013

Owner

I suspect these probably fall under the same "don't expose internal details" criteria as with use_greenlets -- do you have a convincing use case for making these exposed in Flask-PyMongo?

This comment has been minimized.

@behackett

behackett Jan 22, 2013

I'm considering exposing these as read-only properties in PyMongo, only to avoid bug reports like this:
https://jira.mongodb.org/browse/PYTHON-470. For some reason users assume we are doing something wrong, unless we show them we aren't.

@behackett

behackett Jan 22, 2013

I'm considering exposing these as read-only properties in PyMongo, only to avoid bug reports like this:
https://jira.mongodb.org/browse/PYTHON-470. For some reason users assume we are doing something wrong, unless we show them we aren't.

This comment has been minimized.

@ultrabug

ultrabug Jan 24, 2013

I do use those properties on pymongo a lot (and I don't mistake them with the obsolete network_timeout) for certain operations which must be done in a specific time frame or just be ignored. You can imagine this as two different connections like myStandardConnection and myLowLatencyConnection. I often use it in conjunction with a gevent.with_timeout() call as well.

So I imagined I could do the same using flask-pymongo to have a low-latency mongo object for special operations and a standard one.

@behackett : please don't make them read-only ;)

@ultrabug

ultrabug Jan 24, 2013

I do use those properties on pymongo a lot (and I don't mistake them with the obsolete network_timeout) for certain operations which must be done in a specific time frame or just be ignored. You can imagine this as two different connections like myStandardConnection and myLowLatencyConnection. I often use it in conjunction with a gevent.with_timeout() call as well.

So I imagined I could do the same using flask-pymongo to have a low-latency mongo object for special operations and a standard one.

@behackett : please don't make them read-only ;)

This comment has been minimized.

@dcrosta

dcrosta Jan 24, 2013

Owner

I'd be wary of code using socket timeouts like that. Because an operation takes longer than some number of milliseconds to return a result doesn't mean the operation hasn't completed. Imagine you send an update with safe=True (now the default), but wait only 1ms for it to return an answer; the write may still happen, but you will get a timeout in your application, and believe the write hasn't succeeded (or at least not know whether it has). Worse, this will force reconnections, which can have the result of slowing your application even more.

I'm not necessarily opposed to exposing these settings, but I would caution anyone using them for application performance reasons.

@dcrosta

dcrosta Jan 24, 2013

Owner

I'd be wary of code using socket timeouts like that. Because an operation takes longer than some number of milliseconds to return a result doesn't mean the operation hasn't completed. Imagine you send an update with safe=True (now the default), but wait only 1ms for it to return an answer; the write may still happen, but you will get a timeout in your application, and believe the write hasn't succeeded (or at least not know whether it has). Worse, this will force reconnections, which can have the result of slowing your application even more.

I'm not necessarily opposed to exposing these settings, but I would caution anyone using them for application performance reasons.

This comment has been minimized.

@ultrabug

ultrabug Jan 24, 2013

I understand your point, I should have mentioned I use this technique on find() operations only.

I know the operation may still be ongoing on the server side (which is fine because it will have the document fetched and stored on the RAM working set for the next query) but I can't delay my app for more than XXms so I'm fine with not having a result, this is a really eventual find() I'm using this for.

@ultrabug

ultrabug Jan 24, 2013

I understand your point, I should have mentioned I use this technique on find() operations only.

I know the operation may still be ongoing on the server side (which is fine because it will have the document fetched and stored on the RAM working set for the next query) but I can't delay my app for more than XXms so I'm fine with not having a result, this is a really eventual find() I'm using this for.

This comment has been minimized.

@dcrosta

dcrosta Jan 24, 2013

Owner

OK.

Would you mind resubmitting a pull request with just the timeout-related changes, then? I'll accept that. Please also update the docs.

@dcrosta

dcrosta Jan 24, 2013

Owner

OK.

Would you mind resubmitting a pull request with just the timeout-related changes, then? I'll accept that. Please also update the docs.

This comment has been minimized.

@behackett

behackett Jan 25, 2013

@behackett : please don't make them read-only ;)

Sorry, I meant exposing what they were set to as properties on the connection object. We won't take away the ability to set them. That being said, dcrosta is correct that using socket timeouts can cause much larger problems than they solve. Please vote for https://jira.mongodb.org/browse/SERVER-2212 as a better way to time out operations.

@behackett

behackett Jan 25, 2013

@behackett : please don't make them read-only ;)

Sorry, I meant exposing what they were set to as properties on the connection object. We won't take away the ability to set them. That being said, dcrosta is correct that using socket timeouts can cause much larger problems than they solve. Please vote for https://jira.mongodb.org/browse/SERVER-2212 as a better way to time out operations.

This comment has been minimized.

@ultrabug

ultrabug Jan 25, 2013

@behackett : voted thanks, this is indeed exactly what I use those current options for, cheers.

@ultrabug

ultrabug Jan 25, 2013

@behackett : voted thanks, this is indeed exactly what I use those current options for, cheers.

SECONDARY = pymongo.ReadPreference.SECONDARY
"""Distribute queries among replica set secondaries unless none exist or
are up, in which case send queries to the primary."""
SECONDARY_ONLY = pymongo.ReadPreference.SECONDARY_ONLY
SECONDARY_PREFERRED = pymongo.ReadPreference.SECONDARY_PREFERRED

This comment has been minimized.

@dcrosta

dcrosta Jan 22, 2013

Owner

I had initially "copied" these constants here to make the API simpler for Flask-PyMongo users, but I'm now inclined to remove them and force users to import the constants from pymongo directly, to avoid coupling Flask-PyMongo so tightly with the version of pymongo in use. @ajdavis @behackett any opinions here?

@dcrosta

dcrosta Jan 22, 2013

Owner

I had initially "copied" these constants here to make the API simpler for Flask-PyMongo users, but I'm now inclined to remove them and force users to import the constants from pymongo directly, to avoid coupling Flask-PyMongo so tightly with the version of pymongo in use. @ajdavis @behackett any opinions here?

This comment has been minimized.

@behackett

behackett Jan 22, 2013

I would remove these in favor of just using PyMongo's ReadPreference class. Seems silly to have to change flask-pymongo because we added a new option.

@behackett

behackett Jan 22, 2013

I would remove these in favor of just using PyMongo's ReadPreference class. Seems silly to have to change flask-pymongo because we added a new option.

This comment has been minimized.

@ultrabug

ultrabug Jan 24, 2013

This matter seems similar as the write_concern options which are not handled by flask-pymongo.

So I think it's fine to remove them altogether then, else it's confusing tbh (hence the proposal).

@ultrabug

ultrabug Jan 24, 2013

This matter seems similar as the write_concern options which are not handled by flask-pymongo.

So I think it's fine to remove them altogether then, else it's confusing tbh (hence the proposal).

This comment has been minimized.

@ultrabug

ultrabug Jan 25, 2013

@dcrosta : did you have the chance to decide about this matter ? It would be a good candidate for the next tag along the pull-request I just sent maybe :)

@ultrabug

ultrabug Jan 25, 2013

@dcrosta : did you have the chance to decide about this matter ? It would be a good candidate for the next tag along the pull-request I just sent maybe :)

This comment has been minimized.

@dcrosta

dcrosta Jan 25, 2013

Owner

Yes, this would be suitable as a separate pull request. Per the discussion here and elsewhere, we should also remove validation of this parameter, and rely on PyMongo's built-in validation.

@dcrosta

dcrosta Jan 25, 2013

Owner

Yes, this would be suitable as a separate pull request. Per the discussion here and elsewhere, we should also remove validation of this parameter, and rely on PyMongo's built-in validation.

'PRIMARY, SECONDARY, SECONDARY_ONLY (was %r)'
if read_preference not in (NEAREST, PRIMARY, PRIMARY_PREFERRED, SECONDARY, SECONDARY_PREFERRED):
raise Exception('"%s_READ_PREFERENCE" must be one of NEAREST, PRIMARY, '
'PRIMARY_PREFERRED, SECONDARY, SECONDARY_PREFERRED (was %r)'

This comment has been minimized.

@ajdavis

ajdavis Jan 22, 2013

As I've said in a previous, similar discussion, I strongly feel this package shouldn't be duplicating PyMongo's own parameter validation. Do the absolute minimum necessary to get the parameters to PyMongo and let PyMongo do the rest. If a user misconfigures her app and passes a bad value, she'll get a helpful error message from PyMongo. Doing the validation twice is asking for bugs.

@ajdavis

ajdavis Jan 22, 2013

As I've said in a previous, similar discussion, I strongly feel this package shouldn't be duplicating PyMongo's own parameter validation. Do the absolute minimum necessary to get the parameters to PyMongo and let PyMongo do the rest. If a user misconfigures her app and passes a bad value, she'll get a helpful error message from PyMongo. Doing the validation twice is asking for bugs.

This comment has been minimized.

@dcrosta

dcrosta Jan 22, 2013

Owner

@ajdavis I tend to agree. I can't recall why I thought it was wise to do this validation here, rather than relying on PyMongo's validation. I've added #21 to address this.

@dcrosta

dcrosta Jan 22, 2013

Owner

@ajdavis I tend to agree. I can't recall why I thought it was wise to do this validation here, rather than relying on PyMongo's validation. I've added #21 to address this.

except ValueError:
raise TypeError('%s_CONNECT_TIMEOUT_MS must be an integer' % config_prefix)
else:
kwargs['connectTimeoutMS'] = connect_timeout_ms

This comment has been minimized.

@ajdavis

ajdavis Jan 22, 2013

Same comments about validation. I'd prefer to see a config section that just gets passed as kwargs to MongoClient() or MongoReplicaSetClient() with no validation, neither of parameter names nor of their values, for maximum flexibility. PyMongo validates all input params.

@ajdavis

ajdavis Jan 22, 2013

Same comments about validation. I'd prefer to see a config section that just gets passed as kwargs to MongoClient() or MongoReplicaSetClient() with no validation, neither of parameter names nor of their values, for maximum flexibility. PyMongo validates all input params.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta Jan 22, 2013

Owner

@ultrabug after consultation with the experts, I think the right path is to:

  1. Stop "copying" the read preference constants from PyMongo into Flask-PyMongo
  2. Remove validation from class PyMongo, or at least don't add any new validation

I'd be willing to accept a pull request or two which addressed those issues.

I'd also like to hear what your use case for using the timeouts is.

Owner

dcrosta commented Jan 22, 2013

@ultrabug after consultation with the experts, I think the right path is to:

  1. Stop "copying" the read preference constants from PyMongo into Flask-PyMongo
  2. Remove validation from class PyMongo, or at least don't add any new validation

I'd be willing to accept a pull request or two which addressed those issues.

I'd also like to hear what your use case for using the timeouts is.

@ultrabug

This comment has been minimized.

Show comment
Hide comment
@ultrabug

ultrabug Jan 24, 2013

I commented on the related diffs, as for the validation I also agree with you guys to rely on pymongo's direcly (I added those just to be compliant with what seemed to be your current policy on flask-pymongo).

@dcrosta : based on your final observations about the timeouts, I'll be glad to propose a new pull request to address all those matters at once.

Thank you for your help.

ultrabug commented Jan 24, 2013

I commented on the related diffs, as for the validation I also agree with you guys to rely on pymongo's direcly (I added those just to be compliant with what seemed to be your current policy on flask-pymongo).

@dcrosta : based on your final observations about the timeouts, I'll be glad to propose a new pull request to address all those matters at once.

Thank you for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment