From 6596261c307c4824c9ddd6de01faaf27fba2c9b6 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 30 Aug 2022 11:38:50 -0400 Subject: [PATCH 1/6] fix: shuffle servers when there are only two servers --- src/sdam/topology.ts | 4 +--- ...er_selection.prose.operation_count.test.ts | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 69b82befac..03a0627343 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -904,9 +904,7 @@ function processWaitQueue(topology: Topology) { } else if (selectedDescriptions.length === 1) { selectedServer = topology.s.servers.get(selectedDescriptions[0].address); } else { - // don't shuffle the array if there are only two elements - const descriptions = - selectedDescriptions.length === 2 ? selectedDescriptions : shuffle(selectedDescriptions, 2); + const descriptions = shuffle(selectedDescriptions, 2); const server1 = topology.s.servers.get(descriptions[0].address); const server2 = topology.s.servers.get(descriptions[1].address); diff --git a/test/integration/server-selection/server_selection.prose.operation_count.test.ts b/test/integration/server-selection/server_selection.prose.operation_count.test.ts index 48e8c23cb8..7865ce507a 100644 --- a/test/integration/server-selection/server_selection.prose.operation_count.test.ts +++ b/test/integration/server-selection/server_selection.prose.operation_count.test.ts @@ -165,4 +165,27 @@ describe('operationCount-based Selection Within Latency Window - Prose Test', fu expect(percentageToHost1).to.be.greaterThan(40).and.lessThan(60); expect(percentageToHost2).to.be.greaterThan(40).and.lessThan(60); }); + + it( + 'equally distributes operations with both hosts when requests are in parallel', + TEST_METADATA, + async function () { + const collection = client.db('test-db').collection('collection0'); + + const { insertedId } = await collection.insertOne({ name: 'bumpy' }); + + const n = 1000; + + for (let i = 0; i < n; ++i) { + await collection.findOne({ _id: insertedId }); + } + + // Step 9: Using command monitoring events, assert that each mongos was selected roughly 50% of the time (within +/- 10%). + const [host1, host2] = seeds.map(seed => seed.split(':')[1]); + const percentageToHost1 = (counts[host1] / n) * 100; + const percentageToHost2 = (counts[host2] / n) * 100; + expect(percentageToHost1).to.be.greaterThan(40).and.lessThan(60); + expect(percentageToHost2).to.be.greaterThan(40).and.lessThan(60); + } + ); }); From 027454049522045139fc0130c97453d8317576e3 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 31 Aug 2022 13:08:21 -0400 Subject: [PATCH 2/6] chore: remove shuffle in case where 2 servers are selected --- src/sdam/topology.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 03a0627343..4aa9861862 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -903,6 +903,20 @@ function processWaitQueue(topology: Topology) { continue; } else if (selectedDescriptions.length === 1) { selectedServer = topology.s.servers.get(selectedDescriptions[0].address); + } else if (selectedDescriptions.length === 2) { + // If there are only two servers, we avoid shuffling the array of + // server descriptions + const server1 = topology.s.servers.get(selectedDescriptions[0].address); + const server2 = topology.s.servers.get(selectedDescriptions[1].address); + + if (server1?.s.operationCount === server2?.s.operationCount) { + selectedServer = Math.floor(Math.random() * 2) === 0 ? server1 : server2; + } else { + selectedServer = + server1 && server2 && server1.s.operationCount < server2.s.operationCount + ? server1 + : server2; + } } else { const descriptions = shuffle(selectedDescriptions, 2); const server1 = topology.s.servers.get(descriptions[0].address); From ed587241953289387a17ca1d1c1c43bdc2a9ab5a Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 31 Aug 2022 13:43:25 -0400 Subject: [PATCH 3/6] chore: explicitly handle undefined case --- src/sdam/topology.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 4aa9861862..cdca25142d 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -909,13 +909,12 @@ function processWaitQueue(topology: Topology) { const server1 = topology.s.servers.get(selectedDescriptions[0].address); const server2 = topology.s.servers.get(selectedDescriptions[1].address); - if (server1?.s.operationCount === server2?.s.operationCount) { - selectedServer = Math.floor(Math.random() * 2) === 0 ? server1 : server2; - } else { - selectedServer = - server1 && server2 && server1.s.operationCount < server2.s.operationCount - ? server1 - : server2; + if (server1 && server2) { + if (server1.s.operationCount === server2.s.operationCount) { + selectedServer = Math.floor(Math.random() * 2) === 0 ? server1 : server2; + } else { + selectedServer = server1.s.operationCount < server2.s.operationCount ? server1 : server2; + } } } else { const descriptions = shuffle(selectedDescriptions, 2); From 79e3f6d504e185875c4f5358b9d4c0dbcb30b619 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 31 Aug 2022 13:45:46 -0400 Subject: [PATCH 4/6] move comment --- src/sdam/topology.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index cdca25142d..e9bd7025b4 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -904,13 +904,13 @@ function processWaitQueue(topology: Topology) { } else if (selectedDescriptions.length === 1) { selectedServer = topology.s.servers.get(selectedDescriptions[0].address); } else if (selectedDescriptions.length === 2) { - // If there are only two servers, we avoid shuffling the array of - // server descriptions const server1 = topology.s.servers.get(selectedDescriptions[0].address); const server2 = topology.s.servers.get(selectedDescriptions[1].address); if (server1 && server2) { if (server1.s.operationCount === server2.s.operationCount) { + // If there are only two servers, we avoid shuffling the array of + // server descriptions selectedServer = Math.floor(Math.random() * 2) === 0 ? server1 : server2; } else { selectedServer = server1.s.operationCount < server2.s.operationCount ? server1 : server2; From a6176771177e852db7adb6f66d09e0cd40771fab Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 31 Aug 2022 13:47:04 -0400 Subject: [PATCH 5/6] fix test name and add comment about test location --- .../server_selection.prose.operation_count.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/integration/server-selection/server_selection.prose.operation_count.test.ts b/test/integration/server-selection/server_selection.prose.operation_count.test.ts index 7865ce507a..21b41ad423 100644 --- a/test/integration/server-selection/server_selection.prose.operation_count.test.ts +++ b/test/integration/server-selection/server_selection.prose.operation_count.test.ts @@ -167,8 +167,12 @@ describe('operationCount-based Selection Within Latency Window - Prose Test', fu }); it( - 'equally distributes operations with both hosts when requests are in parallel', + 'equally distributes operations with both hosts when requests are in sequence', TEST_METADATA, + /** + * note that this test is NOT a prose test, but it lives in this file because it uses the + * same setup as the operation count prose tests + */ async function () { const collection = client.db('test-db').collection('collection0'); @@ -180,7 +184,6 @@ describe('operationCount-based Selection Within Latency Window - Prose Test', fu await collection.findOne({ _id: insertedId }); } - // Step 9: Using command monitoring events, assert that each mongos was selected roughly 50% of the time (within +/- 10%). const [host1, host2] = seeds.map(seed => seed.split(':')[1]); const percentageToHost1 = (counts[host1] / n) * 100; const percentageToHost2 = (counts[host2] / n) * 100; From f9e53917daba1b197a318da4ea6db9152a1ceea6 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 1 Sep 2022 14:39:09 -0400 Subject: [PATCH 6/6] revert special case for 2 servers --- src/sdam/topology.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index e9bd7025b4..03a0627343 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -903,19 +903,6 @@ function processWaitQueue(topology: Topology) { continue; } else if (selectedDescriptions.length === 1) { selectedServer = topology.s.servers.get(selectedDescriptions[0].address); - } else if (selectedDescriptions.length === 2) { - const server1 = topology.s.servers.get(selectedDescriptions[0].address); - const server2 = topology.s.servers.get(selectedDescriptions[1].address); - - if (server1 && server2) { - if (server1.s.operationCount === server2.s.operationCount) { - // If there are only two servers, we avoid shuffling the array of - // server descriptions - selectedServer = Math.floor(Math.random() * 2) === 0 ? server1 : server2; - } else { - selectedServer = server1.s.operationCount < server2.s.operationCount ? server1 : server2; - } - } } else { const descriptions = shuffle(selectedDescriptions, 2); const server1 = topology.s.servers.get(descriptions[0].address);