From d605e158857a282936af1a9e0e20a2b8b2eca8e6 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 15 Apr 2022 12:04:04 -0400 Subject: [PATCH 1/5] fix: apply localthreshold default of 15ms --- src/sdam/topology_description.ts | 2 +- .../assorted/server_selection.spec.test.ts | 5 ---- test/unit/sdam/topology_description.test.ts | 30 +++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) create mode 100644 test/unit/sdam/topology_description.test.ts diff --git a/src/sdam/topology_description.ts b/src/sdam/topology_description.ts index c9d5736693..34db44f0eb 100644 --- a/src/sdam/topology_description.ts +++ b/src/sdam/topology_description.ts @@ -63,7 +63,7 @@ export class TopologyDescription { this.stale = false; this.compatible = true; this.heartbeatFrequencyMS = options.heartbeatFrequencyMS ?? 0; - this.localThresholdMS = options.localThresholdMS ?? 0; + this.localThresholdMS = options.localThresholdMS ?? 15; if (setName) { this.setName = setName; diff --git a/test/unit/assorted/server_selection.spec.test.ts b/test/unit/assorted/server_selection.spec.test.ts index 2939c546b4..7e81eed3ac 100644 --- a/test/unit/assorted/server_selection.spec.test.ts +++ b/test/unit/assorted/server_selection.spec.test.ts @@ -11,11 +11,6 @@ describe('Server Selection Logic (spec)', function () { this.currentTest.skipReason = 'Nodejs driver does not support PossiblePrimary'; this.skip(); } - - if (this.currentTest.title.match(/nearest_multiple/i)) { - this.currentTest.skipReason = 'TODO(NODE-4188): localThresholdMS should default to 15ms'; - this.skip(); - } }); const selectionSpecDir = join(__dirname, '../../spec/server-selection/server_selection'); diff --git a/test/unit/sdam/topology_description.test.ts b/test/unit/sdam/topology_description.test.ts new file mode 100644 index 0000000000..9d3e74bd44 --- /dev/null +++ b/test/unit/sdam/topology_description.test.ts @@ -0,0 +1,30 @@ +import { expect } from 'chai'; + +import { TopologyType } from '../../../src/sdam/common'; +import { + TopologyDescription, + TopologyDescriptionOptions +} from '../../../src/sdam/topology_description'; + +describe('TopologyDescription (unit)', function () { + describe('#constructor', () => { + context('localThresholdMS', function () { + it('defaults to 15ms', function () { + const description = new TopologyDescription(TopologyType.Single); + expect(description).to.haveOwnProperty('localThresholdMS').to.equal(15); + }); + it('is set when passed in as an option', function () { + const description = new TopologyDescription( + TopologyType.Single, + undefined, + undefined, + undefined, + undefined, + undefined, + { localThresholdMS: 30 } + ); + expect(description).to.haveOwnProperty('localThresholdMS').to.equal(30); + }); + }); + }); +}); From 77f3acc87a8249cc253c11df791b421dc78221e1 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 19 Apr 2022 13:16:32 -0400 Subject: [PATCH 2/5] test: add integration test for localThresholdMS --- .../topology_description.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 test/integration/server-discovery-and-monitoring/topology_description.test.ts diff --git a/test/integration/server-discovery-and-monitoring/topology_description.test.ts b/test/integration/server-discovery-and-monitoring/topology_description.test.ts new file mode 100644 index 0000000000..6c68fb04a7 --- /dev/null +++ b/test/integration/server-discovery-and-monitoring/topology_description.test.ts @@ -0,0 +1,32 @@ +import { expect } from 'chai'; + +import { MongoClient, MongoClientOptions } from '../../../src/mongo_client'; +import { getTopology } from '../../../src/utils'; + +describe('TopologyDescription (integration tests)', function () { + let client: MongoClient; + + afterEach(async function () { + await client.close(); + }); + + context('options', function () { + context('localThresholdMS', function () { + it('should be true', async function () { + const options: MongoClientOptions = {}; + client = await this.configuration.newClient(options).connect(); + const topologyDescription = getTopology(client).description; + expect(topologyDescription).to.have.ownProperty('localThresholdMS').to.equal(15); + }); + + it('should be false', async function () { + const options: MongoClientOptions = { + localThresholdMS: 30 + }; + client = await this.configuration.newClient(options).connect(); + const topologyDescription = getTopology(client).description; + expect(topologyDescription).to.have.ownProperty('localThresholdMS').to.equal(30); + }); + }); + }); +}); From 51523e05abb2eba4995324640c0fbd5a15a04674 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 20 Apr 2022 10:47:16 -0400 Subject: [PATCH 3/5] remove unit tests --- test/unit/sdam/topology_description.test.ts | 30 --------------------- 1 file changed, 30 deletions(-) delete mode 100644 test/unit/sdam/topology_description.test.ts diff --git a/test/unit/sdam/topology_description.test.ts b/test/unit/sdam/topology_description.test.ts deleted file mode 100644 index 9d3e74bd44..0000000000 --- a/test/unit/sdam/topology_description.test.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { expect } from 'chai'; - -import { TopologyType } from '../../../src/sdam/common'; -import { - TopologyDescription, - TopologyDescriptionOptions -} from '../../../src/sdam/topology_description'; - -describe('TopologyDescription (unit)', function () { - describe('#constructor', () => { - context('localThresholdMS', function () { - it('defaults to 15ms', function () { - const description = new TopologyDescription(TopologyType.Single); - expect(description).to.haveOwnProperty('localThresholdMS').to.equal(15); - }); - it('is set when passed in as an option', function () { - const description = new TopologyDescription( - TopologyType.Single, - undefined, - undefined, - undefined, - undefined, - undefined, - { localThresholdMS: 30 } - ); - expect(description).to.haveOwnProperty('localThresholdMS').to.equal(30); - }); - }); - }); -}); From c752d8e434d66afd14c9d09362707848d11e3825 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 20 Apr 2022 10:49:09 -0400 Subject: [PATCH 4/5] fix test descriptions --- .../topology_description.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/topology_description.test.ts b/test/integration/server-discovery-and-monitoring/topology_description.test.ts index 6c68fb04a7..9fdeeab95a 100644 --- a/test/integration/server-discovery-and-monitoring/topology_description.test.ts +++ b/test/integration/server-discovery-and-monitoring/topology_description.test.ts @@ -12,14 +12,14 @@ describe('TopologyDescription (integration tests)', function () { context('options', function () { context('localThresholdMS', function () { - it('should be true', async function () { + it('should default to 15ms', async function () { const options: MongoClientOptions = {}; client = await this.configuration.newClient(options).connect(); const topologyDescription = getTopology(client).description; expect(topologyDescription).to.have.ownProperty('localThresholdMS').to.equal(15); }); - it('should be false', async function () { + it('should be set to the localThresholdMS option when it is passed in', async function () { const options: MongoClientOptions = { localThresholdMS: 30 }; From b3d5923ed2124a29f5da0a200b92f307b72ab39a Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 21 Apr 2022 16:15:08 -0400 Subject: [PATCH 5/5] test: add tests for localThresholdMS Adds tests ensuring that localThresholdMS is respected when calculating servers inside the latency window. --- test/unit/sdam/server_selection.test.js | 73 +++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/test/unit/sdam/server_selection.test.js b/test/unit/sdam/server_selection.test.js index 4f32651be2..03c3188823 100644 --- a/test/unit/sdam/server_selection.test.js +++ b/test/unit/sdam/server_selection.test.js @@ -435,5 +435,78 @@ describe('server selection', function () { }); }); }); + + context('localThresholdMS is respected as an option', function () { + let serverDescription1, serverDescription2, serverDescription3, serverDescriptions; + beforeEach(() => { + serverDescription1 = new ServerDescription( + '127.0.0.1:27017', + { + setName: 'test', + isWritablePrimary: true, + ok: 1 + }, + { roundTripTime: 15 } + ); + serverDescription2 = new ServerDescription( + '127.0.0.1:27018', + { + setName: 'test', + secondary: true, + ok: 1 + }, + { roundTripTime: 25 } + ); + serverDescription3 = new ServerDescription( + '127.0.0.1:27019', + { + setName: 'test', + secondary: true, + ok: 1 + }, + { roundTripTime: 35 } + ); + serverDescriptions = new Map(); + serverDescriptions.set(serverDescription1.address, serverDescription1); + serverDescriptions.set(serverDescription2.address, serverDescription2); + serverDescriptions.set(serverDescription3.address, serverDescription3); + }); + it('includes servers inside the latency window with default localThresholdMS', function () { + const topologyDescription = new TopologyDescription( + TopologyType.Single, + serverDescriptions, + 'test', + MIN_SECONDARY_WRITE_WIRE_VERSION, + new ObjectId(), + MIN_SECONDARY_WRITE_WIRE_VERSION + ); + const selector = secondaryWritableServerSelector(); + const servers = selector(topologyDescription, Array.from(serverDescriptions.values())); + expect(servers).to.have.lengthOf(2); + const selectedAddresses = new Set(servers.map(({ address }) => address)); + expect(selectedAddresses.has(serverDescription1.address)).to.be.true; + expect(selectedAddresses.has(serverDescription2.address)).to.be.true; + expect(selectedAddresses.has(serverDescription3.address)).to.be.false; + }); + + it('includes servers inside the latency window with custom localThresholdMS', function () { + const topologyDescription = new TopologyDescription( + TopologyType.Single, + serverDescriptions, + 'test', + MIN_SECONDARY_WRITE_WIRE_VERSION, + new ObjectId(), + MIN_SECONDARY_WRITE_WIRE_VERSION, + { localThresholdMS: 5 } + ); + const selector = secondaryWritableServerSelector(); + const servers = selector(topologyDescription, Array.from(serverDescriptions.values())); + expect(servers).to.have.lengthOf(1); + const selectedAddresses = new Set(servers.map(({ address }) => address)); + expect(selectedAddresses.has(serverDescription1.address)).to.be.true; + expect(selectedAddresses.has(serverDescription2.address)).to.be.false; + expect(selectedAddresses.has(serverDescription3.address)).to.be.false; + }); + }); }); });