Skip to content

Conversation

@eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Nov 16, 2021

This PR implements CDRIVER-4097, which introduces the srvMaxHosts URI option. This option limits number of hosts during initial DNS seedlist discovery and SRV polling for mongos discovery.

Any changes involving the srvServiceName URI option were deliberately skipped; this feature should be addressed separately as CDRIVER-4086.

This PR comes in three major parts:

  1. URI Options

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.

  1. Initial DNS Seedlist Discovery

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 in dump_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_t respectively, 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 __int128 into 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 numHosts parameter. Evergreen/CI scripts did not have to be updated to accomodate the new sharded spec tests, as the load-balancer spec test setup also happens to satisfy the conditions required to correctly run sharded spec tests (a mongos server on ports 27017 and 27018). These new sharded spec tests can therefore be run alongside existing load-balancer tests on Evergreen.

  1. SRV Polling for mongos Discovery

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 of mongoc_topology_description_reconcile could 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 the MONGOC_TEST_DNS_SRV_POLLING environment variable is set to on. 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.

@eramongodb eramongodb self-assigned this Nov 16, 2021
Copy link
Collaborator

@kevinAlbs kevinAlbs left a 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.

@eramongodb eramongodb requested a review from kevinAlbs November 18, 2021 18:17
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, fantastic work.

Copy link
Contributor

@vector-of-bool vector-of-bool left a 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!

@eramongodb eramongodb merged commit d67527c into mongodb:master Nov 30, 2021
@eramongodb eramongodb deleted the cdriver-4097 branch November 30, 2021 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants