feat: directConnection adds unify behavior for replica set discovery #2349

reggi · 2020-05-04T21:51:47Z

Adds directConnection option to unify behavior around configuration for replica set discovery. Migrated mongodb/specifications tests from commit "e56f5eceed7729f8b9b43a4a1f76c7e5840db49f". Skips SDAM tests for legacy topology / behavior that we do not intend to introduce to the legacy topology types. Users should switch to the unified topology.

NODE-2452

Adds `directConnection` option to unify behavior around configuration for replica set discovery. Migrated mongodb/specifications tests from commit "e56f5eceed7729f8b9b43a4a1f76c7e5840db49f". Skips SDAM tests for legacy topology / behavior that we do not intend to introduce to the legacy topology types. Users should switch to the unified topology. NODE-2452

reggi

[Completed manual review]

reggi

spec changes

lib/sdam/server_selection.js

lib/connection_string.js

lib/sdam/server_selection.js

emadum · 2020-05-05T19:16:44Z

lib/sdam/topology_description.js

+  if (serverType === ServerType.RSGhost || serverType === ServerType.Unknown) {
+    return TopologyType.Unknown;
+  }
+


I personally think switches are cleaner for this sort of thing.

switch (serverType) { case ServerType.Standalone: return TopologyType.single; case ServerType.Mongos: return TopologyType.Sharded; case ServerType.RSPrimary: return TopologyType.ReplicaSetWithPrimary; default: return TopologyType.Unknown; }

Or if we want to keep it exactly the same (void return for unrecognized serverType), replace default: with

case ServerType.RSGhost: case ServerType.Unknown: return TopologyType.Unknown;

Thoughts?

g

reggi · 2020-05-07T17:44:37Z

lib/sdam/topology_description.js

-      return TopologyType.Unknown;
-    default:
+    case ServerType.RSOther:
+    case ServerType.RSSecondary:
      return TopologyType.ReplicaSetNoPrimary;


It didn't make sense to me to have the "default" be ReplicaSetNoPrimary so I swapped this, initial spec tests showed this worked.

nit: I don't find that using a switch here improves readability at all 🤷‍♂️ Maybe adding some line breaks in there would be useful, I just find it hard to visually track

I generally don't prefer switch syntax, but i think @emadum's recommendation makes sense here it does help restrict what this function can do. It makes it more obvious that this function takes a ServerType and returns a TopologyType and no other logic is really allowed.

default Unknown works for me, I'm fine to resolve this comment

test/unit/sdam/spec.test.js

emadum

One question but LGTM 👍

lib/sdam/topology_description.js

mbroadst · 2020-05-11T12:18:14Z

lib/connection_string.js

@@ -37,6 +37,10 @@ function matchesParentDomain(srvAddress, parentDomain) {
 function parseSrvConnectionString(uri, options, callback) {
  const result = URL.parse(uri, true);

+  if (options.directConnection || options.directconnection) {


is it possible to have both forms here? I was hoping we wouldn't be introducing more cases where we had to check the upper and lowercase version of URI options.

During testing it came in as both, I think it performs differently when in URL and passed in, this covers all our bases.

Is there a helper I'm missing? It's also in CASE_TRANSLATION. 😫

so case translation comes into play in applyConnectionStringOption which is in turn called by parseQueryString. Your change here moves the call to parseSrvConnectionString to after parseQueryString is called, which means that we should be in a world where all known values are camelCased.

@reggi I think we're waiting on you here

mbroadst · 2020-05-11T12:19:45Z

lib/mongo_client.js

@@ -143,6 +143,7 @@ const { connect, validOptions } = require('./operations/connect');
 * @param {boolean} [options.useNewUrlParser=true] Determines whether or not to use the new url parser. Enables the new, spec-compliant, url parser shipped in the core driver. This url parser fixes a number of problems with the original parser, and aims to outright replace that parser in the near future. Defaults to true, and must be explicitly set to false to use the legacy url parser.
 * @param {DriverInfoOptions} [options.driverInfo] Allows a wrapping driver to amend the client metadata generated by the driver to include information about the wrapping driver
 * @param {AutoEncrypter~AutoEncryptionOptions} [options.autoEncryption] Optionally enable client side auto encryption
+ * @param {boolean} [options.directConnection=false] Enable directConnection


can we improve this doc string? "Indicates that a client should directly connect to a node without attempting to discover its topology type" or something like that

mbroadst · 2020-05-11T12:19:57Z

lib/mongo_client.js

@@ -388,6 +389,7 @@ MongoClient.prototype.isConnected = function(options) {
 * @param {number} [options.numberOfRetries=5] The number of retries for a tailable cursor
 * @param {boolean} [options.auto_reconnect=true] Enable auto reconnecting for single server instances
 * @param {number} [options.minSize] If present, the connection pool will be initialized with minSize connections, and will never dip below minSize connections
+ * @param {boolean} [options.directConnection=false] Enable directConnection


same comment

mbroadst · 2020-05-11T12:21:14Z

lib/sdam/topology.js

@@ -101,6 +101,10 @@ class Topology extends EventEmitter {
   * @param {number} [options.localThresholdMS=15] The size of the latency window for selecting among multiple suitable servers
   * @param {number} [options.serverSelectionTimeoutMS=30000] How long to block for server selection before throwing an error
   * @param {number} [options.heartbeatFrequencyMS=10000] The frequency with which topology updates are scheduled
+   * @param {boolean} [options.directConnection] will set topology to Single
+   * @param {string} [options.replicaSet] will set topology to ReplicaSetNoPrimary


replicaSet should be the only supported option, setName and rs_name can and should be removed in this major revision. Also I think the docstrings for all of these are a bit wanting, directConnection means "connect directly to the node without discovering its type" and "replicaSet" is the name of the replica set to connect to (used for validation during initial handshake as well). You can find decent descriptions of these in the URI Options spec

mbroadst · 2020-05-11T12:27:18Z

lib/sdam/topology_description.js

-      return TopologyType.Unknown;
-    default:
+    case ServerType.RSOther:
+    case ServerType.RSSecondary:
      return TopologyType.ReplicaSetNoPrimary;


nit: I don't find that using a switch here improves readability at all 🤷‍♂️ Maybe adding some line breaks in there would be useful, I just find it hard to visually track

test/unit/sdam/spec.test.js

lib/sdam/topology_description.js

mbroadst

LGTM!

Thomas Reggi added 2 commits May 4, 2020 17:51

Merge branch 'master' into NODE-2452/master/directConnection

ae14f1c

reggi commented May 5, 2020

View reviewed changes

reggi requested review from mbroadst and emadum May 5, 2020 16:25

reggi commented May 5, 2020

View reviewed changes

lib/sdam/server_selection.js Outdated Show resolved Hide resolved

lib/sdam/server_selection.js Outdated Show resolved Hide resolved

mbroadst reviewed May 5, 2020

View reviewed changes

lib/connection_string.js Outdated Show resolved Hide resolved

lib/sdam/server_selection.js Outdated Show resolved Hide resolved

emadum reviewed May 5, 2020

View reviewed changes

changes

0a50a18

reggi force-pushed the NODE-2452/master/directConnection branch from f381018 to 0a50a18 Compare May 7, 2020 00:09

Thomas Reggi added 5 commits May 6, 2020 20:24

lint fix"

29bb331

g

rename topologyTypeFromSeedlist to topologyTypeFromOptions

67a5dda

skips Monitoring a standalone connection tests

a67f83c

added switch for topologyTypeForServerType

ad68403

remove default ReplicaSetNoPrimary

e14501c

reggi commented May 7, 2020

View reviewed changes

test/unit/sdam/spec.test.js Show resolved Hide resolved

reggi requested review from emadum and mbroadst May 7, 2020 18:32

emadum approved these changes May 7, 2020

View reviewed changes

lib/sdam/topology_description.js Show resolved Hide resolved

mbroadst suggested changes May 11, 2020

View reviewed changes

reccos from matt

4b5dedc

reggi requested a review from mbroadst May 11, 2020 21:27

fixes mongo srv spec test

c1ee288

reggi force-pushed the NODE-2452/master/directConnection branch from 665942d to c1ee288 Compare May 13, 2020 22:26

mbroadst approved these changes May 14, 2020

View reviewed changes

reggi merged commit 34c9195 into master May 15, 2020

reggi deleted the NODE-2452/master/directConnection branch May 15, 2020 15:58

louisnow mentioned this pull request Sep 22, 2021

Mongoose can't connect to mongodb 4.4.4 replica set inside a docker container Automattic/mongoose#10718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: directConnection adds unify behavior for replica set discovery #2349

feat: directConnection adds unify behavior for replica set discovery #2349

reggi commented May 4, 2020

reggi left a comment •

edited

Loading

reggi left a comment

emadum May 5, 2020 •

edited

Loading

reggi May 7, 2020

mbroadst May 11, 2020

reggi May 11, 2020

mbroadst May 12, 2020

emadum left a comment

mbroadst May 11, 2020

reggi May 11, 2020

mbroadst May 12, 2020

mbroadst May 13, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

mbroadst left a comment

feat: directConnection adds unify behavior for replica set discovery #2349

feat: directConnection adds unify behavior for replica set discovery #2349

Conversation

reggi commented May 4, 2020

reggi left a comment • edited Loading

Choose a reason for hiding this comment

reggi left a comment

Choose a reason for hiding this comment

emadum May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emadum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbroadst left a comment

Choose a reason for hiding this comment

reggi left a comment •

edited

Loading

emadum May 5, 2020 •

edited

Loading