- Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-4097 Implement srvMaxHosts for initial DNS seedlist and SRV polling #898
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
kevinAlbs left a comment
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.
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 left a comment
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, fantastic work.
vector-of-bool left a comment
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.
Sorry for the very slow review. This looks good. The thorough inline comments are very helpful!
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.