-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-2672 SDAM, CMAP, and server selection changes for load balancers #621
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
Disable SRV Polling. Disable SDAM compatibility check. Disable logicalSessionTimeoutMinutes check. Disable server session pool pruning. Disable server selection. Disable Monitors. A ServerType of LoadBalancer MUST be considered a data-bearing server. "drivers MUST emit the following series of SDAM events" section. Send loadBalanced:True with handshakes, validate serviceId. Add topologyVersion fallback when serviceId is missing. Don't mark load balancers unknown.
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
pymongo/client_options.py
Outdated
@@ -140,7 +140,8 @@ def _parse_pool_options(options): | |||
appname, | |||
driver, | |||
compression_settings, | |||
server_api=server_api) | |||
server_api=server_api, | |||
load_balanced=options.get('loadbalanced')) |
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.
Any reason we are pulling out the other options like server_api
into a local variable instead of reading it directly from the options dictionary like we are doing for loadbalanced
? If not, can we do a quick refactor here and get rid of all the pointless code that just get
s the option values?
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 adjusted it to define a local variable. There's no reason to do one or the other so I opted to use the existing style.
How did you test this locally or on EVG? Is there something you can point me to? |
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.
Changes LGTM. I am curious to know how you are testing this but it can wait for the testing PR. As long as you have tested it somehow locally, I am fine with merging this as-is.
I've tested this both locally and on EVG. Testing PR will be up today or tomorrow. For now see my comment about setting up a local LB on https://jira.mongodb.org/browse/PYTHON-2542. |
…rs (#621) Disable SRV Polling, SDAM compatibility check, logicalSessionTimeoutMinutes check. server session pool pruning, server selection, and server monitoring. A ServerType of LoadBalancer MUST be considered a data-bearing server. "drivers MUST emit the following series of SDAM events" section. Send loadBalanced:True with handshakes, validate serviceId. Add topologyVersion fallback when serviceId is missing. Don't mark load balancers unknown. (cherry picked from commit 5bf15c8)
This implements most of the load balancers spec changes: https://github.com/mongodb/specifications/blob/master/source/load-balancers/load-balancers.rst#server-discovery-and-monitoring
The notable things left out are testing and the various Connection Pooling changes:
I'm still working my way through testing. It is a bit involved because we need to update the retryable writes, reads, crud, change streams, uri options, transactions, SDAM, server selection, and unified format spec tests. Including going from schema version 1.1 to 1.4. I've decided to split that to a separate PR.