PYTHON-2791 Ignore erroneous serviceId field for non-LB connections #663
Conversation
|
If this is a serverless bug should add a comment that the special-casing needs to be removed once serverless fixes the problem upstream? |
|
On a second thought, lets leave this open for the time being. The code change is small enough that I can manually copy it to get unblocked on serverless testing. |
|
I'd like to merge this instead of wait for serverless testing. I've added a test. We can remove the pool logic in PYTHON-2712 when we remove the other serviceId related code. |
| @@ -361,7 +361,7 @@ def updated_topology_description(topology_description, server_description): | |||
| topology_description._topology_settings) | |||
|
|
|||
| if topology_type == TOPOLOGY_TYPE.Unknown: | |||
| if server_type == SERVER_TYPE.Standalone: | |||
| if server_type in (SERVER_TYPE.Standalone, SERVER_TYPE.LoadBalancer): | |||
There was a problem hiding this comment.
So with multiple seeds + LB we will now end up removing hosts until we have only one left and then connect to that as a Single?
Is this what the spec says to do or is this behavior undefined?
There was a problem hiding this comment.
See the comment in the test case. This is unreachable code in practice. It's just a failsafe in case the "serviceId" popping logic doesn't work or gets removed prematurely.
| self.assertEqual(t.description.topology_type_name, 'Single') | ||
| self.assertTrue(t.description.has_writable_server()) | ||
|
|
||
| # Load balancers are removed from a topology with multiple seeds. |
There was a problem hiding this comment.
What if both a and b advertise a serviceId?
There was a problem hiding this comment.
They'd both get removed. No need to test it though since this is unreachable in practice. We don't need to test every combination IMO.
There was a problem hiding this comment.
My reading of the code tells me that if the serviceId popping fails, we'll be left with one LB to which we'll connect as a standalone but I take your point that this is unreachable - we can leave it at that.
|
To clarify, is the plan to revert this change entirely as part of PYTHON-2712? If so, can you make a note about the same on the ticket? |
The serviceId field is not supposed to be returned for non-LB mode. This change fixes the pymongo bug in the ticket by removing the field (if it exists) when in non-LB mode.