SPEC-1555 Consider connection pool health during server selection #876

patrickfreed · 2020-11-04T21:04:20Z

This PR updates the Server Selection algorithm to consider connection pool health as per the design in DRIVERS-781. It also updates the CMAP spec to require liveness checking of pooled connections.

A first draft of this PR was filed on my fork while the maxConnecting changes were still in flux. See the initial rounds of discussion there: patrickfreed#1

patrickfreed · 2020-11-06T21:33:15Z

Due to discussion on the other draft PR, I updated the "health" determination to include the WaitQueue length of a particular pool. This will help in situations where all pools are full and have WaitQueues, and it will also help in situations where a non-full pool has developed a WaitQueue due to waiting on maxConnecting.

source/server-selection/server-selection.rst

p-mongo · 2020-11-09T18:56:55Z

I suggest adding rationales/descriptions for:

Prioritizing even load distribution across servers over reusing connections
Prioritizing even load distribution across servers once connection counts have reached steady state, even at the expense of exacerbating connection storms when the application or database is started or restarted (note: previous spec change to cmap limited how many concurrent connections can be outstanding to one node from one client to mitigate the storms)

ShaneHarvey

I've implemented the tests and they are passing as expected. A few comments on the test format.

source/server-selection/server-selection.rst

ShaneHarvey · 2020-11-09T21:08:04Z

source/server-selection/tests/README.rst

+  ``in_window`` array and values are numbers in [0, 1] indicating the frequency
+  at which the server should have been selected.
+
+For each file, pass the information from `in_window` to whatever function is


The test should also configure the read preference used for server selection. I suggest adding a read_preference field to be consistent with the existing tests.

I think we discussed this in the meeting, but since this test is verifying what happens after the set of available servers is determined and the topologies will always be sharded, a read preference shouldn't be necessary here.

Hmm maybe my test runner implementation is different. In mine the read preference does actually have an impact. Right now I'm arbitrarily using Nearest. My implementation is roughly:

topology = create_mock_topology_from(test['topology_description']) mock_operation_counts(topology, test['in_window']) # Mock operationCount for each server in 'in_window' read_preference = Nearest() counts = ... for _ in range(ITERATIONS): server = topology.select_server(read_preference) counts[server.address] += 1 ...

Ah I see, yeah that makes sense. In my implementation I skipped right to the in window portion, so I didn't need a TopologyDescription or a read preference. I imagine most drivers will probably implement your way though.

Rather than including a read preference in each spec test, I just updated the runner's description to say use a default or "primary" read preference since it shouldn't matter for sharded topologies.

It still feels a little odd that we have no explicit tests for replica sets. Perhaps the prose test can cover it?

The server selection logic tests already ensure that drivers properly determine what is in the latency window across different topology types, and these tests ensure that a server is selected properly from what's within the latency window. Putting those together should provide us full coverage, so I don't think it's necessary to test all topologies in these tests.

Given that selecting from within the window is topology agnostic, the topology is provided in these tests purely as a convenience for implementing the test runners; it doesn't really have any bearing on the results of the tests.

On the other hand It's not unreasonable for a driver to have two code paths for server selection and adding one test for a replica set is trivial, so why not add a single replica set test? The test format supports it as long as we say to use Nearest read preference instead of Primary.

My main hesitation for adding it was that if we're treating the provided topology as more than just a convenience (i.e. as part of the tests' coverage), then drivers would be required to use it to implement the tests, even though it isn't necessary since the feature we're testing should ideally be topology agnostic. That said, if we think there could be separate implementations for selection based on topology, then it's probably worth having full coverage of that. Updated to include a few replset tests.

source/server-selection/tests/README.rst

also only require multithreaded drivers to use new algorithm

patrickfreed

This PR has been rewritten to consolidate the various counts we retrieved from the pool to a single operationCount that is incremented after a server is selected and decremented once the operation it was selected for is completed. Additionally, the consideration of availableConnectionCount was dropped, so no changes to or interaction with CMAP is required for this work. This should make the changes here much simpler and easier to understand.

patrickfreed · 2020-11-10T23:18:06Z

source/server-selection/tests/README.rst

+  ``in_window`` array and values are numbers in [0, 1] indicating the frequency
+  at which the server should have been selected.
+
+For each file, pass the information from `in_window` to whatever function is


I think we discussed this in the meeting, but since this test is verifying what happens after the set of available servers is determined and the topologies will always be sharded, a read preference shouldn't be necessary here.

patrickfreed · 2020-11-10T23:19:21Z

source/server-selection/tests/README.rst

@@ -65,3 +65,37 @@ the TopologyDescription. Each YAML file contains a key for these stages of serve
 Drivers implementing server selection MUST test that their implementation
 correctly returns the set of servers in ``in_latency_window``. Drivers SHOULD also test
 against ``suitable_servers`` if possible.
+
+Selection Within Latency Window Tests


Now that the "algorithm" is much simpler, is it still worth it to have all drivers implement these unit tests?

I think it's still worthwhile. I very much prefer these tests to no tests. That said, do you have anything in mind for real end-to-end tests for this feature?

We're limited a bit by the topology. We could include some of those experiments I did as prose tests--for example, describe a test against a two mongos sharded topology where one of the mongoses has a failpoint that makes every operation take 500ms, then do a ton of concurrent stuff and then assert that the non-failpoint node got picked a lot more. While this does verify the operationCount behavior, I'm not sure whether this is preferable to having a bunch of unit spec tests, though.

Please open a jira ticket so we can further discuss adding that prose test. I'm in favor of it.

Should I just go ahead and add it now? I think it'll be most useful if drivers can have the test case when they're implementing this for the first time.

Yeah let's just add it now. I doubt the unified test format is expressive enough for a test like this so it'll need to be a prose test like you said. You can use the blockConnection option for failCommand like this:

db.adminCommand({ configureFailPoint: "failCommand", mode: {times: 100000}, # or "alwaysOn" data: { failCommands: ["find"], blockConnection: true, blockTimeMS: 500, appName: "loadBalancingTest", }, });

done. When you run it, can you let me know what you get for % of operations routed to the slow node? Curious to see if theres any differences across drivers.

Implemented. I ran it a few times and see:

{('localhost', 27017): 11, ('localhost', 27018): 89} {('localhost', 27017): 12, ('localhost', 27018): 88} {('localhost', 27017): 13, ('localhost', 27018): 87} {('localhost', 27017): 15, ('localhost', 27018): 85} {('localhost', 27017): 17, ('localhost', 27018): 83}

Exciting to see this feature in action!

I get around 12-15% too, so it seems like our implementations are consistent, nice!

source/server-selection/server-selection.rst

source/server-selection/tests/README.rst

source/server-selection/server-selection.rst

patrickfreed · 2020-11-10T23:35:31Z

Also, our discussions made me realize these changes have no effect on single threaded drivers, so I updated the spec to require this only for multi-threaded or async drivers.

ShaneHarvey

Also, our discussions made me realize these changes have no effect on single threaded drivers, so I updated the spec to require this only for multi-threaded or async drivers.

Interesting, this is in line with our scope which says:

[Non-goals]:
Change the behavior of single-threaded drivers
Single-threaded driver instances do not maintain connection pools and thus are unlikely to cause connection storms on their own

A quick note that if single threaded drivers begin supporting any feature that require connection pinning (like OP_MSG exhaust cursors), then they will actually need to implement operationCount.

source/server-selection/server-selection.rst

ShaneHarvey · 2020-11-11T01:34:51Z

source/server-selection/tests/README.rst

@@ -65,3 +65,37 @@ the TopologyDescription. Each YAML file contains a key for these stages of serve
 Drivers implementing server selection MUST test that their implementation
 correctly returns the set of servers in ``in_latency_window``. Drivers SHOULD also test
 against ``suitable_servers`` if possible.
+
+Selection Within Latency Window Tests


I think it's still worthwhile. I very much prefer these tests to no tests. That said, do you have anything in mind for real end-to-end tests for this feature?

ShaneHarvey · 2020-11-11T01:38:20Z

source/server-selection/tests/README.rst

+  ``in_window`` array and values are numbers in [0, 1] indicating the frequency
+  at which the server should have been selected.
+
+For each file, pass the information from `in_window` to whatever function is


Hmm maybe my test runner implementation is different. In mine the read preference does actually have an impact. Right now I'm arbitrarily using Nearest. My implementation is roughly:

topology = create_mock_topology_from(test['topology_description']) mock_operation_counts(topology, test['in_window']) # Mock operationCount for each server in 'in_window' read_preference = Nearest() counts = ... for _ in range(ITERATIONS): server = topology.select_server(read_preference) counts[server.address] += 1 ...

source/server-selection/server-selection.rst

p-mongo

The spec changes look great, thank you.

I have not attempted to implement the tests.

mbroadst

Spec changes look great, I'll LGTM but we should hold off on merge until Shane validates the tests

source/server-selection/server-selection-tests.rst

source/server-selection/server-selection.rst

ShaneHarvey · 2020-11-12T16:46:35Z

source/server-selection/server-selection.rst

+                    selected = in_window[0]
+                else:
+                    server1, server2 = random two entries from in_window
+                    if server1.operation_count >= server2.operation_count:


This should be <=.

fixed, thank you for catching this! This would've been a pretty bad error to leave in there.

source/server-selection/tests/in_window/three-choices.yml

source/server-selection/tests/in_window/maxConnecting-waitQueue.yml

ShaneHarvey · 2020-11-12T18:07:49Z

source/server-selection/tests/README.rst

@@ -65,3 +65,37 @@ the TopologyDescription. Each YAML file contains a key for these stages of serve
 Drivers implementing server selection MUST test that their implementation
 correctly returns the set of servers in ``in_latency_window``. Drivers SHOULD also test
 against ``suitable_servers`` if possible.
+
+Selection Within Latency Window Tests


Please open a jira ticket so we can further discuss adding that prose test. I'm in favor of it.

source/server-selection/server-selection.rst

Co-authored-by: Matt Broadstone <mbroadst@gmail.com>

source/server-selection/server-selection.rst

patrickfreed · 2020-11-12T20:51:06Z

source/server-selection/server-selection.rst

+                    selected = in_window[0]
+                else:
+                    server1, server2 = random two entries from in_window
+                    if server1.operation_count >= server2.operation_count:


fixed, thank you for catching this! This would've been a pretty bad error to leave in there.

patrickfreed · 2020-11-12T20:52:09Z

source/server-selection/tests/README.rst

@@ -65,3 +65,37 @@ the TopologyDescription. Each YAML file contains a key for these stages of serve
 Drivers implementing server selection MUST test that their implementation
 correctly returns the set of servers in ``in_latency_window``. Drivers SHOULD also test
 against ``suitable_servers`` if possible.
+
+Selection Within Latency Window Tests


Should I just go ahead and add it now? I think it'll be most useful if drivers can have the test case when they're implementing this for the first time.

source/server-selection/tests/in_window/three-choices.yml

source/server-selection/tests/in_window/maxConnecting-waitQueue.yml

patrickfreed · 2020-11-12T21:41:22Z

source/server-selection/tests/in_window/many-choices.yml

@@ -0,0 +1,60 @@
+description: Selections from many choices occur at correct frequencies


@ShaneHarvey

This test includes some really small frequencies, so I upped the iteration count to 10,000 and decreased the threshold to 2%. Let me know if this works for your runner okay and doesn't take too long to run.

Hmm 10000 makes the tests take ~300ms each vs ~50ms for 2000 iterations which is pretty slow for a unit test. What if we made this configurable per test?:

in_window: ... outcome: iterations: 10000 tolerance: 0.02 expected_frequencies: a:27017: 0.22 b:27017: 0.18 c:27017: 0.18 d:27017: 0.125 e:27017: 0.125 f:27017: 0.074 g:27017: 0.074 h:27017: 0.0277 i:27017: 0

I think this would be a nice solution, but the false negative you got in the other thread makes me worried about 2% tolerance with 10k iterations.

So we can still implement this proposal, let's just increase outcome.tolerance for this test and/or reduce the number of servers in the topology.

Increased tolerance to 0.03, which seems to work fine for me. Let me know if that has any issues for your runner.

Note: I left iterations outside of outcome since it had more to do with running the test then inspecting the outcome. tolerance is under there though.

ShaneHarvey · 2020-11-12T21:48:34Z

source/server-selection/tests/in_window/many-choices.yml

@@ -0,0 +1,60 @@
+description: Selections from many choices occur at correct frequencies


Hmm 10000 makes the tests take ~300ms each vs ~50ms for 2000 iterations which is pretty slow for a unit test. What if we made this configurable per test?:

in_window: ... outcome: iterations: 10000 tolerance: 0.02 expected_frequencies: a:27017: 0.22 b:27017: 0.18 c:27017: 0.18 d:27017: 0.125 e:27017: 0.125 f:27017: 0.074 g:27017: 0.074 h:27017: 0.0277 i:27017: 0

source/server-selection/tests/in_window/two-choices.yml

source/server-selection/tests/in_window/one-least-two-tied.json

- include iterations and tolerance - require the usage of the topology description - add tests for replica sets - rename in_window to mocked_topology_state

ShaneHarvey

LGTM with a minor suggestion!

ShaneHarvey · 2020-11-13T23:01:58Z

source/server-selection/server-selection-tests.rst

+6. Disable the failpoint.
+
+7. Repeat this test without any failpoints and assert that each mongos was
+   selected roughly 50% of the time.


Can you add roughly 50% of the time within +/-10% tolerance.? I initially implemented this with 5% but that failed with: AssertionError: 0.55 != 0.5 within 0.05 delta (0.050000000000000044 difference)

source/server-selection/tests/README.rst

Co-authored-by: Matt Broadstone <mbroadst@gmail.com>

patrickfreed · 2020-11-16T23:55:36Z

@mbroadst @ShaneHarvey
I updated the part about where the operationCount is stored, let me know what you all think.

mbroadst · 2020-11-17T00:29:14Z

@patrickfreed looks good!

…ngodb#876)

spec changes

5c71cda

patrickfreed requested review from mbroadst and ShaneHarvey November 4, 2020 21:04

patrickfreed requested review from divjotarora and p-mongo as code owners November 4, 2020 21:04

patrickfreed mentioned this pull request Nov 4, 2020

SPEC-1555 Consider connection pool health during server selection patrickfreed/specifications#1

Closed

divjotarora removed their request for review November 5, 2020 15:53

incorporate waitQueue length when calculating load

a0a2f8d

fix tests

e8ef656

p-mongo reviewed Nov 9, 2020

View reviewed changes

source/server-selection/server-selection.rst Outdated Show resolved Hide resolved

ShaneHarvey requested changes Nov 9, 2020

View reviewed changes

patrickfreed added 3 commits November 10, 2020 17:54

use operationCount instead of pool info in selection

2a824d0

also only require multithreaded drivers to use new algorithm

revert changes to cmap spec

ecdd538

fix typos

1c4c4ad

patrickfreed commented Nov 10, 2020

View reviewed changes

ShaneHarvey reviewed Nov 11, 2020

View reviewed changes

patrickfreed added 3 commits November 11, 2020 15:54

update rationale

92e9f3d

include readpreference guidance

488fa8b

add reference to algorithm back

4e6a6f3

p-mongo approved these changes Nov 12, 2020

View reviewed changes

mbroadst approved these changes Nov 12, 2020

View reviewed changes

source/server-selection/server-selection-tests.rst Outdated Show resolved Hide resolved

source/server-selection/server-selection.rst Outdated Show resolved Hide resolved

ShaneHarvey reviewed Nov 12, 2020

View reviewed changes

ShaneHarvey requested changes Nov 12, 2020

View reviewed changes

ShaneHarvey mentioned this pull request Nov 12, 2020

PYTHON-2395 Consider connection pool health during server selection mongodb/mongo-python-driver#515

Merged

2 tasks

patrickfreed and others added 3 commits November 12, 2020 15:47

Update source/server-selection/server-selection-tests.rst

6aa42d0

Co-authored-by: Matt Broadstone <mbroadst@gmail.com>

mention operationCount could be stored on pool

c833255

fix typo in pseudocode

3236ee5

add new test cases, increase iteration count, decrese freq leeway

1945eb9

patrickfreed commented Nov 12, 2020

View reviewed changes

ShaneHarvey reviewed Nov 12, 2020

View reviewed changes

source/server-selection/tests/in_window/one-least-two-tied.json Show resolved Hide resolved

patrickfreed added 5 commits November 12, 2020 17:37

remove extra host from test

8df3608

introduce e2e prose test

0caf78a

update server-selection-tests.rst

74c6299

move prose test to server-selection-tests.rst

960131a

update test format

73be550

- include iterations and tolerance - require the usage of the topology description - add tests for replica sets - rename in_window to mocked_topology_state

ShaneHarvey approved these changes Nov 13, 2020

View reviewed changes

add guidance for what "roughly" means

e88135f

mbroadst reviewed Nov 16, 2020

View reviewed changes

source/server-selection/tests/README.rst Outdated Show resolved Hide resolved

patrickfreed and others added 2 commits November 16, 2020 16:04

fix typo

59772b8

Co-authored-by: Matt Broadstone <mbroadst@gmail.com>

remove SHOULD from operationCount storage location

af823a6

ShaneHarvey approved these changes Nov 17, 2020

View reviewed changes

patrickfreed merged commit 07790d1 into mongodb:master Nov 17, 2020

kevinAlbs pushed a commit to kevinAlbs/specifications that referenced this pull request Nov 9, 2021

SPEC-1555 Consider connection pool health during server selection (mo…

4a3ade7

…ngodb#876)

		@@ -0,0 +1,60 @@
		description: Selections from many choices occur at correct frequencies

Navigation Menu

SPEC-1555 Consider connection pool health during server selection #876

SPEC-1555 Consider connection pool health during server selection #876

Conversation

patrickfreed commented Nov 4, 2020

patrickfreed commented Nov 6, 2020 • edited

p-mongo commented Nov 9, 2020

ShaneHarvey left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed commented Nov 10, 2020

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-mongo left a comment

Choose a reason for hiding this comment

mbroadst left a comment

Choose a reason for hiding this comment

ShaneHarvey Nov 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed commented Nov 16, 2020

mbroadst commented Nov 17, 2020

patrickfreed commented Nov 6, 2020 •

edited

ShaneHarvey left a comment •

edited

ShaneHarvey Nov 12, 2020 •

edited