-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3247 Mitigate user issues caused by change in directConnection defaults in 4.x #935
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
Conversation
pymongo/mongo_client.py
Outdated
| """Client for a MongoDB instance, a replica set, or a set of mongoses. | ||
| .. warning:: Starting in PyMongo 4.0, ``directConnection`` now has a default value of | ||
| False instead of None (which is the same as specifying ``directionConnection=True``). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"(which is the same as specifying directionConnection=True)" is not quite correct. Let's remove that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bernie mentioned that fact explicitly in the ticket, so I thought it would be good to include so that user understands that None does not equal False (as I would assume). What about making it more clear by saying that is "functionally" the same? Or is that still not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directConnection=None is not supported in PyMongo 4:
>>> c = MongoClient(directConnection=None)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 747, in __init__
dict(common.validate(keyword_opts.cased_key(k), v) for k, v in keyword_opts.items())
File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 747, in <genexpr>
dict(common.validate(keyword_opts.cased_key(k), v) for k, v in keyword_opts.items())
File "/Users/shane/git/mongo-python-driver/pymongo/common.py", line 754, in validate
value = validator(option, value)
File "/Users/shane/git/mongo-python-driver/pymongo/common.py", line 185, in validate_boolean_or_string
return validate_boolean(option, value)
File "/Users/shane/git/mongo-python-driver/pymongo/common.py", line 176, in validate_boolean
raise TypeError("%s must be True or False" % (option,))
TypeError: directConnection must be True or False
In PyMongo 3 directConnection=None means: use the legacy pymongo 3 discovery behavior (ie 1 host -> direct, >1 host -> auto discover).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok that makes sense. Will leave it removed then.
doc/changelog.rst
Outdated
| discovery of replica sets. This means that if you | ||
| want a direct connection to a single server you must pass | ||
| ``directConnection=True`` as a URI option or keyword argument. | ||
| See the :doc:`migrate-to-pymongo4` for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we link to the directConnection section in the migration guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
doc/migrate-to-pymongo4.rst
Outdated
| doc = client.admin.command('hello') | ||
| result = doc['isWritablePrimary'] | ||
| print(result) | ||
| >> True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The >> True seems a little odd. How about?:
>>> client.admin.command('hello')['isWritablePrimary']
True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
doc/migrate-to-pymongo4.rst
Outdated
| want a direct connection to a single server you must pass | ||
| ``directConnection=True`` as a URI option or keyword argument. | ||
|
|
||
| Here are some example errors that you might see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding something like "If you see any ServerSelectionTimeoutError after upgrading from 3 to 4, you likely need to add directConnection=True when creating the client. Here are some example errors:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, done.
| defaults to ``False`` instead of ``None``, allowing for the automatic | ||
| discovery of replica sets. This means that if you | ||
| want a direct connection to a single server you must pass | ||
| ``directConnection=True`` as a URI option or keyword argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this in the list here too just for more visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongo/mongo_client.py
Outdated
| Please see the :ref:`pymongo4-migration-guide` for more details. | ||
| False instead of None. | ||
| For more details, see the relevant section of the PyMongo 4.x migration guide: | ||
| :ref:`pymongo4-migration-direct-connection`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are under-indented now: https://readthedocs.org/projects/pymongo/builds/16807087/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, my bad. Copy and pasted it wrong. Fixed.
ShaneHarvey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one small change
doc/migrate-to-pymongo4.rst
Outdated
| .. code-block:: | ||
| _select_servers_loop | ||
| raise ServerSelectionTimeoutError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove these two lines:
_select_servers_loop
raise ServerSelectionTimeoutError(
… defaults in 4.x (mongodb#935)
No description provided.