feat(NODE-3467): implement srvMaxHosts, srvServiceName options #3031

nbbeeken · 2021-11-08T22:06:04Z

Description

What is changing?

A new option srvMaxHosts is now implemented that enables a user to limit the number of mongoses the driver will connect to. The driver will select from the list of hosts returned by a DNS lookup at random using the Fisher-Yates algorithm.

The srvServiceName allows the srv address prefix to be changed from the default of mongodb

Is there new documentation needed for these changes?

There is new TSDoc, I don't think further documentation will be needed.

What is the motivation for this change?

The number of mongos hosts can be large and each gets its own connection pool, it allows for tuning of how many query routers a driver connects to, primarily in order to manage networking resources.

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

dariakp

Some feedback just on the helpers

test/unit/utils.test.js

src/utils.ts

test/unit/utils.test.js

src/utils.ts

test/unit/utils.test.js

src/sdam/srv_polling.ts

dariakp

More feedback on just the shuffle portion

test/unit/utils.test.js

src/utils.ts

test/unit/utils.test.js

…IntervalMS options

dariakp

just some wording clean up on the shuffle

test/unit/utils.test.js

src/utils.ts

Co-authored-by: Daria Pardue <[email protected]>

make use of timers and await IO

dariakp

Just capturing some thoughts before I move on to looking at more of this

src/mongo_client.ts

src/connection_string.ts

src/sdam/srv_polling.ts

dariakp

Thoughts on srvMaxHosts specifically

src/connection_string.ts

dariakp

More notes

src/sdam/topology_description.ts

src/utils.ts

dariakp

Still have tests to review, but here's more feedback

test/unit/utils.test.js

src/connection_string.ts

dariakp

Some feedback on just the code, maybe hopefully the last round, still need to look at the tests

src/utils.ts

src/connection_string.ts

dariakp

URI spec test portion comments

test/unit/mongo_client.test.js

test/functional/uri_options_spec.test.js

durran · 2021-11-12T09:10:56Z

test/unit/connection_string.test.js

+              // TODO(): srvMaxHost limiting happens in the callback given to resolveSRVRecord inside the driver
+              // How can we test this in unit test?
+
+              // if (options.srvHost && comment.includes('srvMaxHosts')) {


Just a note but it's potentially one way to add a check... Could be that we test that the list of hosts after the randomisation is equal to the list of server description host names in the created topology? Just thinking aloud.

Moved these tests to initial_dns_seedlist_discovery.spec.test.ts to go along with our nice naming convention but also skipped them due to needing more concrete assertions around the error cases. And it is tracked here: NODE-3757

Co-authored-by: Daria Pardue <[email protected]>

nbbeeken · 2021-11-15T16:26:47Z

@dariakp I went through and made sure I addresses all your current comments.
@durran I've rewritten the commented out testing in connection_string.test.js, its close to working, I just need to do so much manual address lookups in the tests to make sure we get the same results expected by the tests. So everything expected is there, we just have slightly different shaped data.

nbbeeken · 2021-11-15T19:30:25Z

src/sdam/topology.ts

+    const selectedHosts =
+      options.srvMaxHosts === 0 || options.srvMaxHosts >= seedlist.length
+        ? seedlist
+        : shuffle(seedlist, options.srvMaxHosts);


I moved the initial shuffling to here, in order to unit test since the callback to resolveSRVRecord is inaccessible to the tests.

seems like that works too

dariakp

Some comments while I look at the tests

src/utils.ts

test/unit/mongo_client.test.js

src/operations/connect.ts

Co-authored-by: Daria Pardue <[email protected]>

dariakp

Just to summarize what we discussed - we want to pull in the spec changes associated with test 13, which involve NODE-3450 along with the changes that would have been done in NODE-3680; we also want to make the mock tests more closely resemble the prose tests as written, and we want to break out the seed discovery tests into their own file and see if they can run as integration tests.

test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts

test/unit/mongo_client.test.js

test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts

durran

LGTM and fine with implementing NODE-3757 later.

dariakp self-requested a review November 9, 2021 15:13

dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 9, 2021

dariakp requested changes Nov 9, 2021

View reviewed changes

test/unit/utils.test.js Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

nbbeeken commented Nov 9, 2021

View reviewed changes

src/utils.ts Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

src/sdam/srv_polling.ts Outdated Show resolved Hide resolved

dariakp requested changes Nov 10, 2021

View reviewed changes

test/unit/utils.test.js Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

nbbeeken added 6 commits November 10, 2021 14:10

feat(NODE-3467): implement srvMaxHosts, srvServiceName, and rescanSrv…

ce4d7e4

…IntervalMS options

fix: unit tests

6321246

fix: integration tests

52b3377

wip

676f8f3

test: fix up shuffle unit tests

7879773

fix: shuffle tests round 2 undo srv event saving

1d09e18

nbbeeken force-pushed the NODE-3467/limit-mongos branch from b74b277 to 1d09e18 Compare November 10, 2021 19:11

dariakp requested changes Nov 10, 2021

View reviewed changes

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

test/unit/utils.test.js Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

nbbeeken and others added 7 commits November 10, 2021 14:31

Apply suggestions from code review

f7c304d

Co-authored-by: Daria Pardue <[email protected]>

test: remove dupe test

eca1ee4

fix: remove unused equals method

471dc61

docs: improve limit description

0b35275

fix: lint

5daa3ce

fix: remove rescan option and drop TXT record option logic

382ad4d

fix: permit new options only on srv connection strings

852fca3

make use of timers and await IO

dariakp requested changes Nov 10, 2021

View reviewed changes

src/mongo_client.ts Outdated Show resolved Hide resolved

src/mongo_client.ts Outdated Show resolved Hide resolved

src/connection_string.ts Show resolved Hide resolved

src/connection_string.ts Outdated Show resolved Hide resolved

src/sdam/srv_polling.ts Outdated Show resolved Hide resolved

dariakp requested changes Nov 10, 2021

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

dariakp requested changes Nov 10, 2021

View reviewed changes

src/sdam/topology_description.ts Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

dariakp requested changes Nov 10, 2021

View reviewed changes

test/unit/utils.test.js Outdated Show resolved Hide resolved

src/connection_string.ts Outdated Show resolved Hide resolved

src/connection_string.ts Outdated Show resolved Hide resolved

nbbeeken added 3 commits November 11, 2021 11:52

fix: address comments, fix option parsing errors, test naming

28567f8

fix: LB connection string assertion

00a39d1

feat: super algorithm enhancements O(-1) speeds

12a72ab

dariakp requested changes Nov 11, 2021

View reviewed changes

test/unit/mongo_client.test.js Show resolved Hide resolved

test/functional/uri_options_spec.test.js Outdated Show resolved Hide resolved

test/functional/uri_options_spec.test.js Outdated Show resolved Hide resolved

durran reviewed Nov 12, 2021

View reviewed changes

nbbeeken and others added 3 commits November 12, 2021 10:50

fix: shuffle lowerBound logic, test for srvServiceName length error

5fa6550

or -> nor

537ec45

Co-authored-by: Daria Pardue <[email protected]>

fix: address comments except for connection_string tests

0db7bb8

nbbeeken requested a review from dariakp November 15, 2021 16:24

fix: whoops broke host gathering, fixed now

42a0942

nbbeeken commented Nov 15, 2021

View reviewed changes

dariakp requested changes Nov 15, 2021

View reviewed changes

suggestions!

0db9494

Co-authored-by: Daria Pardue <[email protected]>

dariakp requested changes Nov 15, 2021

View reviewed changes

test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts Outdated Show resolved Hide resolved

nbbeeken added 5 commits November 15, 2021 17:48

move tests into correct places, update initial seed list testing WIP

60e3c75

add ticket mention

34bc0d4

clarify records

beb74c5

remove comment

0e45319

prevent mutation

d9e8f28

dariakp requested changes Nov 16, 2021

View reviewed changes

test/unit/mongo_client.test.js Show resolved Hide resolved

test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts Outdated Show resolved Hide resolved

test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts Outdated Show resolved Hide resolved

nbbeeken added 2 commits November 16, 2021 09:10

test: add object option test, clean up assertions

eec372e

call makeStubs first in test 13

7465308

dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Nov 16, 2021

dariakp marked this pull request as ready for review November 16, 2021 14:59

fix: check for nullish srvMaxHosts

ad521aa

dariakp approved these changes Nov 16, 2021

View reviewed changes

nbbeeken requested a review from durran November 16, 2021 17:01

fix: increase the input size

09b30d6

durran approved these changes Nov 16, 2021

View reviewed changes

dariakp merged commit 1f8b539 into main Nov 16, 2021

dariakp deleted the NODE-3467/limit-mongos branch November 16, 2021 20:05

avlan mentioned this pull request Nov 26, 2021

fix(NODE-3767): don't delete dbName if authSource is provided #3055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-3467): implement srvMaxHosts, srvServiceName options #3031

feat(NODE-3467): implement srvMaxHosts, srvServiceName options #3031

nbbeeken commented Nov 8, 2021

dariakp left a comment

dariakp left a comment

dariakp left a comment

dariakp left a comment

dariakp left a comment

dariakp left a comment

dariakp left a comment

dariakp left a comment

dariakp left a comment

durran Nov 12, 2021

nbbeeken Nov 16, 2021

nbbeeken commented Nov 15, 2021

nbbeeken Nov 15, 2021

dariakp Nov 15, 2021

dariakp left a comment

dariakp left a comment

durran left a comment

feat(NODE-3467): implement srvMaxHosts, srvServiceName options #3031

feat(NODE-3467): implement srvMaxHosts, srvServiceName options #3031

Conversation

nbbeeken commented Nov 8, 2021

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

durran Nov 12, 2021

Choose a reason for hiding this comment

nbbeeken Nov 16, 2021

Choose a reason for hiding this comment

nbbeeken commented Nov 15, 2021

nbbeeken Nov 15, 2021

Choose a reason for hiding this comment

dariakp Nov 15, 2021

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

durran left a comment

Choose a reason for hiding this comment