CDRIVER-4097 Implement srvMaxHosts for initial DNS seedlist and SRV polling#898
Merged
eramongodb merged 27 commits intomongodb:masterfrom Nov 30, 2021
Merged
CDRIVER-4097 Implement srvMaxHosts for initial DNS seedlist and SRV polling#898eramongodb merged 27 commits intomongodb:masterfrom
eramongodb merged 27 commits intomongodb:masterfrom
Conversation
kevinAlbs
reviewed
Nov 18, 2021
Collaborator
kevinAlbs
left a comment
There was a problem hiding this comment.
This looks great. Thank you for the detailed description. The prose test refactoring of pooled/single functions is a nice pattern. The only substantial comments are about the new random functions.
If the warning can be disabled or removed, the implementation of mongoc_topology_description_reconcile could be made simpler.
The warning is motivated by an SDAM specification recommendation in CDRIVER-3107.
kevinAlbs
approved these changes
Nov 18, 2021
Collaborator
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM, fantastic work.
vector-of-bool
approved these changes
Nov 30, 2021
Contributor
vector-of-bool
left a comment
There was a problem hiding this comment.
Sorry for the very slow review. This looks good. The thorough inline comments are very helpful!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements CDRIVER-4097, which introduces the
srvMaxHostsURI option. This option limits number of hosts during initial DNS seedlist discovery and SRV polling for mongos discovery.Any changes involving the
srvServiceNameURI option were deliberately skipped; this feature should be addressed separately as CDRIVER-4086.This PR comes in three major parts:
First, a bug in the validation of load balancer URI options had to be addressed to permit valid execution of
srvMaxHosts-related tests.Then, the necessary URI option parsing features were implemented and corresponding spec tests added to the uri-options directory. Note that some test cases had to be added to the warning filters as CDRIVER-3167 has not yet been addressed.
There were two drive-by improvements during this step. The first was the addition of a
const-qualifier to a read-only host list function. The second was fixing an infinite loop bug indump_hosts()(only triggered by certain test failures) due to an incorrect induction step.As the Initial Seedlist Discovery Spec requires random shuffling and selection of hosts, I took the opportunity to introduce a set of uniform random integer functions. These are
_mongoc_rand_uint32_t,_mongoc_rand_uint64_t, and_mongoc_rand_size_trespectively, each returning a random integer in the range[min, max]with uniform distribution. The algorithms used are as described in "Fast Random Integer Generation in an Interval" by Daniel Lemire. The "nearly divisionless" algorithm proposed by the paper is used for 32-bit integers. The "Java algorithm" is used for 64-bit integers as the nearly divisionless algorithm requires using 128-bit integers during computation; I did not want to attempt to introduce__int128into the C driver due to consistent cross-platform compatibility being effectively non-existent.With the new random number generators, implementing the filtering of initial DNS seedlist hosts was relatively straightforward.
The new spec tests for Initial Seedlist Discovery also required some modification to test code to accomodate the new
numHostsparameter. Evergreen/CI scripts did not have to be updated to accomodate the newshardedspec tests, as theload-balancerspec test setup also happens to satisfy the conditions required to correctly runshardedspec tests (a mongos server on ports 27017 and 27018). These newshardedspec tests can therefore be run alongside existingload-balancertests on Evergreen.There was some awkward math required to preserve the correct order of operations, where the addition of new valid hosts (within the limit as described by
srvMaxHosts) must come before removal of missing hosts to avoid generating a misleading warning. If the warning can be disabled or removed, the implementation ofmongoc_topology_description_reconcilecould be made simpler.Finally, prose tests 10, 11, and 12 were added according to the SRV Polling Tests Spec. However, due to the verbosity and repetition of the existing patterns used by prose tests, I took the opportunity to perform some dependency inversion (or is it "Inversion of Control"? Either way, some kind of "inversion" is involved) to simplify the implementation of the prose test code. To better understand the intent of this refactor, I recommend comparing the state of a single prose test before and after the change in separate views, rather than side-by-side or inline as a diff.
The SRV Polling prose tests have been given the
test_dns_check_srv_polling()skip condition such that they are only run if theMONGOC_TEST_DNS_SRV_POLLINGenvironment variable is set toon. This is due to current Evergreen scripts not yet being configured to create the required sharded cluster configuration with mongos instances running on 4 ports. Supporting this will take additional work as described by CDRIVER-4230. Note, this means that the Evergreen tasks being run for this PR do not currently evaluate the SRV Polling prose tests, unliked the sharded tests for Initial DNS Seedlist Discovery, although they have been verified locally as passing given the correct sharded cluster configuration.