RUBY-2746 RUBY-2851 RUBY-2737 Provide options to limit number of mongos servers used in connecting to sharded clusters #2380

neilshweky · 2021-12-09T20:02:35Z

Adding ticket RUBY-2851 to this PR. This ticket is for removing the DNS spec tests that conflict with url-options spec tests.
Also adding RUBY-2737 to this PR. This ticket is for adding the srvServiceName option, and is completely implemented by this ticket.

…ection

p-mongo

Is there a test that specifying srv_max_hosts actually reduces the number of seeds compared to what is in the DNS? Apologies if I missed it, if not it should be added.

p-mongo · 2021-12-13T06:53:10Z

lib/mongo/client.rb

@@ -1226,6 +1228,12 @@ def validate_new_options!(opts)
            end

            _options[key] = compressors unless compressors.empty?
+          elsif key == :srv_max_hosts
+            if v && (!v.is_a?(Integer) || v < 0)
+              log_warn("#{v} is not a valid integer for srvMaxHosts")


srv_max_hosts is I would say more appropriate here.

p-mongo · 2021-12-13T06:53:55Z

lib/mongo/client.rb

@@ -1239,7 +1247,7 @@ def validate_new_options!(opts)
    # Validates all options after they are set on the client.
    # This method is intended to catch combinations of options which are
    # not allowed.
-    def validate_options!(addresses = nil)
+    def validate_options!(addresses = nil, is_srv = nil)


Please use the keyword argument syntax for is_srv: is_srv: nil.

p-mongo · 2021-12-13T06:54:54Z

lib/mongo/client.rb

+
+      if options[:srv_max_hosts] && options[:srv_max_hosts] > 0
+        if options[:replica_set]
+          raise ArgumentError, ":srv_max_hosts > 0 cannot be used with replica_set option"


The options should all be referenced the same way, i.e. either with leading colons or without (preferably in the way that is most similar to other validators).

p-mongo · 2021-12-13T06:55:11Z

lib/mongo/client.rb

+
+      unless is_srv.nil? || is_srv
+        if options[:srv_max_hosts]
+          raise ArgumentError, "srvMaxHosts cannot be used on non-SRV URI"


srv_max_hosts

p-mongo · 2021-12-13T06:57:16Z

lib/mongo/client.rb

@@ -92,6 +92,8 @@ class Client
      :server_api,
      :server_selection_timeout,
      :socket_timeout,
+      :srv_max_hosts,
+      :srv_service_name,


These options should be documented on the Client constructor. If they are used only internally by the driver, they should be annotated as such.

This still needs to be done right?

p-mongo · 2021-12-13T07:01:03Z

lib/mongo/srv/resolver.rb

+      # @param [ String | nil ] srv_service_name The SRV service name for the DNS query.
+      #   If nil, 'mongodb' is used.
+      # @param [ Integer | nill ] srv_max_hosts The maximum number of records to return.
+      #   If this value is zero or nil, return all of the records.


I think it would be better to change zero to nil sometime during client construction and everywhere downstream of the client require and expect (with validation, if necessary) that the value is either nil or a positive integer.

lib/mongo/uri.rb

p-mongo · 2021-12-13T07:05:05Z

lib/mongo/uri.rb

+      # byebug
+      if uri_options[:srv_max_hosts] && uri_options[:srv_max_hosts] > 0
+        if uri_options[:replica_set]
+          raise_invalid_error_no_fmt!(":srv_max_hosts > 0 cannot be used with replica_set option")


These messages should use the URI option names, e.g. srvMaxHosts, replicaSet.

p-mongo · 2021-12-13T07:06:33Z

spec/mongo/client_construction_spec.rb

+          end
+        end
+
+        context 'srv_max_hosts > 0 and replicaSet' do


replica_set, please check the naming of other options in this file too.

neilshweky · 2021-12-13T16:54:31Z

Is there a test that specifying srv_max_hosts actually reduces the number of seeds compared to what is in the DNS? Apologies if I missed it, if not it should be added.

There are spec tests that do it (srvMaxHosts-less_than_srv_records.yml) but I didn't write any Ruby tests. Should I?

…nil on 0

p-mongo · 2021-12-13T17:27:54Z

The srvMaxHosts-less_than_srv_records.yml test doesn't assert the resulting seed list, therefore I expect it to pass if the driver e.g. happens to replace the seed list with a single bogus value. Looking at the test runner implementation, as far as I can tell that test would also be silently not performed if the file format changed (as opposed to producing an error due to unexpected test format).

Based on this, I suggest adding a test which uses one of the test URIs, create a client with it, then inspect the seed list and verify that the seed is one of expected values.

neilshweky · 2021-12-13T17:55:43Z

The srvMaxHosts-less_than_srv_records.yml test doesn't assert the resulting seed list, therefore I expect it to pass if the driver e.g. happens to replace the seed list with a single bogus value. Looking at the test runner implementation, as far as I can tell that test would also be silently not performed if the file format changed (as opposed to producing an error due to unexpected test format).

Based on this, I suggest adding a test which uses one of the test URIs, create a client with it, then inspect the seed list and verify that the seed is one of expected values.

It's difficult to assert the actual seedlist, since if max hosts is less than the number of hosts, it selects a random subset of hosts of size srv_max_hosts. I can assert that the hosts that it does come up with are valid, though.

EDIT: done!

p-mongo · 2021-12-14T14:06:16Z

docs/reference/create-client.txt

+     - 0
+
+   * - ``:srv_service_name``
+     - The SRV service name to use in the DNS query.


I think "The service name to use in the SRV DNS query" would be more clear.

p-mongo · 2021-12-14T14:08:10Z

docs/reference/create-client.txt

+       for sharded topologies. If this option is set to 0, there will
+       be no maximum number of connections. If the given URI resolves
+       to more hosts than ``:srv_max_hosts``, the client will ramdomly
+       choose an ``:srv_max_hosts`` sized subset of hosts.


I would add here a note that the hosts that the driver ignores during client construction will never be used, i.e., if a deployment has 10 total mongoses and this option limits them to 5, and those 5 happen to become unavailable, the client will quit working completely even though the deployment overall still has 5 functional mongoses elsewhere.

p-mongo · 2021-12-14T14:09:23Z

docs/reference/create-client.txt

+     - The maximum number of mongos connections that may be created
+       for sharded topologies. If this option is set to 0, there will


I would adjust the first sentence to something like "The maximum number of mongoses that the driver will communicate with", because the number of connections is controlled by pool size and can exceed 1 per mongos.

p-mongo · 2021-12-14T14:10:27Z

lib/mongo/client.rb

@@ -92,6 +92,8 @@ class Client
      :server_api,
      :server_selection_timeout,
      :socket_timeout,
+      :srv_max_hosts,
+      :srv_service_name,


This still needs to be done right?

p-mongo · 2021-12-14T14:12:00Z

spec/mongo/uri/srv_protocol_spec.rb

+          let(:options) { '/?srvMaxHosts=1' }
+          it 'returns an array with only one of the parsed servers' do
+            expect(uri.servers.length).to eq 1
+            expect(hosts.include?(uri.servers.first)).to be true


comandeo

👍

neilshweky added 12 commits December 8, 2021 11:46

RUBY-2746 add spec tests

bcd3e95

RUBY-2746 add another spec test

3dc5c1d

RUBY-2746 implemented srvServiceName, 5 tests still failing

6a5db78

RUBY-2746 added srvMaxHosts implementation and fix test for it

8320891

RUBY-2746 add type that raises error on no parse, ALL TESTS PASSING

27512a9

RUBY-2746 move uri validation for replica set

1aa46f6

RUBY-2746 uri options passing, still waiting on bad input failure dir…

b4355fb

…ection

RUBY-2746 remove offending tests and integer_raise

fcb17dc

RUBY-2746 clean up old test files and combine runners

6690762

RUBY-2746 add support for ruby options

3b95e74

RUBY-2746 add tests for ruby options

74be568

RUBY-2746 cleanup comments

698878f

p-mongo reviewed Dec 13, 2021

View reviewed changes

neilshweky added 2 commits December 13, 2021 12:08

RUBY-2746 fix tests, naming, method signatures, set srv_max_hosts to …

65d5caf

…nil on 0

RUBY-2746 don't set srv_max_hosts on 0

b3ff177

RUBY-2746 add docs for new options

7806cf7

neilshweky changed the title ~~RUBY-2746 Provide options to limit number of mongos servers used in connecting to sharded clusters~~ RUBY-2746 RUBY-2851 Provide options to limit number of mongos servers used in connecting to sharded clusters Dec 13, 2021

RUBY-2746 fix bug, add test for srv_max_hosts

0e27498

neilshweky requested a review from p-mongo December 13, 2021 18:49

neilshweky added 2 commits December 13, 2021 14:40

RUBY-2746 add tests for different valus of srvMaxHosts

f88d678

RUBY-2746 add srvServiceName test

44f8c53

neilshweky changed the title ~~RUBY-2746 RUBY-2851 Provide options to limit number of mongos servers used in connecting to sharded clusters~~ RUBY-2746 RUBY-2851 RUBY-2737 Provide options to limit number of mongos servers used in connecting to sharded clusters Dec 13, 2021

neilshweky added 3 commits December 13, 2021 15:42

RUBY-2746 fix spec tests, make srv_max_hosts accept 0

b4ec4b3

RUBY-2746 fix srv tests, disregard sorting

ef9b376

RUBY-2746 fix tests sorting

8e240b4

p-mongo reviewed Dec 14, 2021

View reviewed changes

RUBY-2746 update comments and docs

a1a2308

neilshweky requested a review from p-mongo December 14, 2021 15:13

p-mongo approved these changes Dec 14, 2021

View reviewed changes

neilshweky requested a review from comandeo December 14, 2021 17:45

comandeo approved these changes Dec 15, 2021

View reviewed changes

neilshweky merged commit 4f5e06d into mongodb:master Dec 15, 2021

		- The maximum number of mongos connections that may be created
		for sharded topologies. If this option is set to 0, there will

RUBY-2746 RUBY-2851 RUBY-2737 Provide options to limit number of mongos servers used in connecting to sharded clusters #2380

RUBY-2746 RUBY-2851 RUBY-2737 Provide options to limit number of mongos servers used in connecting to sharded clusters #2380

Uh oh!

Conversation

neilshweky commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p-mongo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neilshweky commented Dec 13, 2021

Uh oh!

p-mongo commented Dec 13, 2021

Uh oh!

neilshweky commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comandeo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neilshweky commented Dec 9, 2021 •

edited

Loading

neilshweky commented Dec 13, 2021 •

edited

Loading