NODE-1259 Refactor mongodb-core to use a single Topology type (part 1) #303

mbroadst · 2018-04-30T21:44:07Z

This PR introduces a single Topology type meant to completely replace Server/Replset/Mongos in both mongodb-core and node-mongodb-native. Presently it is not hooked up at all to the driver, but passes all SDAM, server selection and max-staleness tests. The next step will be to actually connect the internal pieces so we can effectively replace the existing topologies. (Broken apart to give a glimmer of a chance at reviewing)

This is the core definition of a single Topology type meant to replace the existing `Mongos` and `Replset` types. It presently passes all SDAM tests, however is not yet hooked up to use the existing `Server` type for internal representation of servers. NODE-1259

This adds an incomplete, but sufficient amount of SDAM monitoring to the new Topology type in order to pass the SDAM monitoring YAML tests. NODE-1259

NODE-1259

The server selection test runner is nearly the same as that used for max staleness tests. I've included both into the same file, with an eye towards refactoring the runner out in the future.

This adds a `maxStalenessReducer` to our server selectors, and adds it in all the right places in the `readPreferenceServerSelector`.

To prevent breaking changes, we will still support passing in tags as an object rather than the documented required array. This will change in 4.x

daprahamian

Preliminary feedback

daprahamian · 2018-04-30T21:49:55Z

lib/error.js

+ * @return {MongoTimeoutError} A MongoTimeoutError instance
+ * @extends {MongoError}
+ */
+const MongoTimeoutError = function(message) {


Can this be done with class and extends?

yes, but I was just following existing convention in the file. I'd prefer to convert them in a separate commit wholesale

daprahamian · 2018-04-30T21:54:09Z

test/tests/functional/replset_server_selection_tests.js

@@ -98,7 +98,12 @@ function executeEntry(file, path) {
    });

    // Create read preference
-    var rp = new ReadPreference(convert(readPreference.mode), readPreference.tag_sets);
+    var rp;


daprahamian · 2018-04-30T22:05:23Z

lib/sdam/topology_description.js

+const MAX_SUPPORTED_WIRE_VERSION = 5;
+
+// An enumeration of topology types we know about
+const TopologyType = {


['Single', 'ReplicaSetNoPrimary', 'ReplicaSetWithPrimary', 'Sharded', 'Unknown'].reduce((memo, key) => { memo[key] = key; return memo; }, {});

tbh that feels way less readable to me for the sake of reducing very limited (and unlikely to ever change) text duplication

daprahamian · 2018-04-30T22:05:54Z

lib/sdam/topology_description.js

+   * @param {number} maxSetVersion
+   * @param {ObjectId} maxElectionId
+   */
+  constructor(topologyType, serverDescriptions, setName, maxSetVersion, maxElectionId, options) {


We should POJO this constructor.

I feel like this request keeps coming up in code reviews, is there a particular reason you want to do that beyond readability? This is an internal type to boot, so I'm not sure what it buys us.

It's better from a maintenance perspective. You can see a bunch of the benefits of RORO here. We don't get everything b/c we still have Node 4, but from a maintenance perspective it is really useful.

Also, from past experience, I have been burned by not following RORO.

I dunno, I'm not convinced. Should we make a class called TopologyUpdateParameters? I think it's a little overkill just for the benefit of having named parameters. The named parameters example in that article is also not very compelling - agreed, definitely don't use "magic" boolean parameters. In this case, we are very clearly passing in named variables, essentially giving us the same benefit. On top of that, requiring an extra allocation of an object just to call a function seems needlessly wasteful. I we can discuss POJOs for the future, but I don't think this PR is the place to make that choice

daprahamian · 2018-04-30T22:06:43Z

lib/sdam/topology_description.js

+        this.compatible = false;
+        this.compatibilityError = `Server at ${serverDescription.address} requires wire version ${
+          serverDescription.minWireVersion
+        }, but this version of the driver only supports up to ${MAX_SUPPORTED_WIRE_VERSION}.`;


should there be a break after this?

not according to spec

daprahamian · 2018-04-30T22:52:30Z

lib/sdam/topology.js

+          ? TopologyType.ReplicaSetNoPrimary
+          : TopologyType.Unknown;
+
+    const topologyId = globalTopologyCounter++;


Can we make a topologyCounter()?

what do you mean by this? a function on the class itself? a package-local variable seems fine to me, what would we gain otherwise?

I just don't like the idea of incrementing the globalTopologyCounter inline. I'd rather that we wrap it in a topologyCounter function that internally handles it.

daprahamian · 2018-04-30T22:53:00Z

lib/sdam/topology.js

+    const topologyId = globalTopologyCounter++;
+
+    const serverDescriptions = seedlist.reduce((result, seed) => {
+      const address = seed.port ? `${seed.host}:${seed.port}` : `${seed.host}:27017`;


Lets make an address(...) function.

to be used only here?

yeah, just to clean it up.

daprahamian · 2018-04-30T22:56:51Z

lib/sdam/topology.js

+      options.serverSelectionTimeoutMS,
+      process.hrtime(),
+      (err, servers) => {
+        if (err) return callback(err, null);


Is there a reason we needed to wrap the callback in this arrow function?

I don't follow?

Ah, I missed randomSelection. Nvm.

daprahamian · 2018-04-30T22:57:58Z

lib/sdam/topology.js

+   * @param {authResultCallback} callback A callback function
+   */
+  auth(mechanism, db, options, callback) {
+    callback(null, null);


Is this not implemented yet?

yes these are all stubs right now

daprahamian · 2018-04-30T22:58:07Z

lib/sdam/topology.js

+   * @param {authResultCallback} callback A callback function
+   */
+  logout(db, callback) {
+    callback(null, null);


yes these are all stubs right now

mbroadst added 22 commits April 30, 2018 17:41

refactor(sdam): track logicalSessionTimeoutMinutes from ismasters

e029583

test(sdam): skip monitoring tests for the moment

6948e33

chore(sdam): create sdam subfolder, split topology up

ac80267

feat(sdam-monitoring): add basic monitoring for new Topology type

80ad079

This adds an incomplete, but sufficient amount of SDAM monitoring to the new Topology type in order to pass the SDAM monitoring YAML tests. NODE-1259

fix(topology-description): we can't use Object.values yet

fe1be0d

fix(sdam): we can't use Array.includes yet

a7d0e64

refactor(topology-description): use Maps for server descriptions

f5e5590

refactor(calculateDurationMs): move this method to common utils

f2f08f2

feat(MongoTimeoutError): add common class for timeout events

f01e4c8

refactor(read-preference): simplify arguments management

a31dfd1

feat(server-selection): add basic support for server selection

92d1c30

NODE-1259

test(server-selection): add server selection test runner

22f87f5

refactor(latency-window): ensure use of lowest bound for minimum

ca3b831

test(latency-window): modify selection tests to verify latency

0db8da2

refactor(server-description): add lastUpdateTime, fix spelling

cc2e369

refactor(topology-description): add getter for common wire version

7438829

test(max-staleness): reuse server selection test runner

73157a6

The server selection test runner is nearly the same as that used for max staleness tests. I've included both into the same file, with an eye towards refactoring the runner out in the future.

feat(max-staleness): properly support a max staleness reducer

c4aec15

This adds a `maxStalenessReducer` to our server selectors, and adds it in all the right places in the `readPreferenceServerSelector`.

refactor(read-preference): support legacy non-array for tags

bb619f0

To prevent breaking changes, we will still support passing in tags as an object rather than the documented required array. This will change in 4.x

test(max-staleness): correct old test runners for redpref changes

ff2dcf0

refactor(topology): add stubs for all base topology methods

6f07c7c

daprahamian suggested changes Apr 30, 2018

View reviewed changes

mbroadst added 7 commits May 1, 2018 09:32

feat(topology-description): add helper method for server ownership

aa3642f

refactor(topology): add Server type, add serverClosed events

8fe2bf7

test(sdam): reenable removal test, testing serverClosed events

4da938c

chore(sdam): Server skeleton takes a description for testing

7995834

refactor(server-description): simplify ismaster parsing

135de8c

test(sdam): accommodate for and verify omitted fields

c00ffc0

refactor(servrer-selectors): simplify nested ternary statement

198f43f

mbroadst added 3 commits May 2, 2018 16:14

refactor(topology): simplify default options, type detection

fb5d72a

refactor(topology-description): remove assert, dedupe if branches

31eb08a

test(server-selection): var => let

9653cb1

mbroadst merged commit 38d5f40 into master May 23, 2018

mbroadst deleted the NODE-1259 branch May 23, 2018 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NODE-1259 Refactor mongodb-core to use a single Topology type (part 1) #303

NODE-1259 Refactor mongodb-core to use a single Topology type (part 1) #303

mbroadst commented Apr 30, 2018

daprahamian left a comment

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian Apr 30, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian May 1, 2018

mbroadst May 2, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian May 1, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian May 1, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian May 2, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

daprahamian Apr 30, 2018

mbroadst May 1, 2018

NODE-1259 Refactor mongodb-core to use a single Topology type (part 1) #303

NODE-1259 Refactor mongodb-core to use a single Topology type (part 1) #303

Conversation

mbroadst commented Apr 30, 2018

daprahamian 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

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

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