Skip to content

Commit 451b98f

Browse files
nbbeekennbbeeken
committed
fix(NODE-3712): SDAM should give priority to electionId over setVersion when updating topology
1 parent 8970ac1 commit 451b98f

File tree

6 files changed

+186
-41
lines changed

6 files changed

+186
-41
lines changed

‎src/sdam/topology_description.ts‎

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Document, ObjectId } from '../bson';
1+
import type { ObjectId } from '../bson';
22
import * as WIRE_CONSTANTS from '../cmap/wire_protocol/constants';
33
import { MongoError, MongoRuntimeError } from '../error';
44
import { shuffle } from '../utils';
@@ -364,25 +364,9 @@ function topologyTypeForServerType(serverType: ServerType): TopologyType {
364364
}
365365
}
366366

367-
// TODO: improve these docs when ObjectId is properly typed
368-
function compareObjectId(oid1: Document, oid2: Document): number {
369-
if (oid1 == null) {
370-
return -1;
371-
}
372-
373-
if (oid2 == null) {
374-
return 1;
375-
}
376-
377-
if (oid1.id instanceof Buffer && oid2.id instanceof Buffer) {
378-
const oid1Buffer = oid1.id;
379-
const oid2Buffer = oid2.id;
380-
return oid1Buffer.compare(oid2Buffer);
381-
}
382-
383-
const oid1String = oid1.toString();
384-
const oid2String = oid2.toString();
385-
return oid1String.localeCompare(oid2String);
367+
function compareObjectId(oid1: ObjectId, oid2: ObjectId): 0 | 1 | -1 {
368+
const res = oid1.id.compare(oid2.id);
369+
return res === 0 ? 0 : res > 0 ? 1 : -1;
386370
}
387371

388372
function updateRsFromPrimary(
@@ -398,12 +382,12 @@ function updateRsFromPrimary(
398382
return [checkHasPrimary(serverDescriptions), setName, maxSetVersion, maxElectionId];
399383
}
400384

401-
const electionId = serverDescription.electionId ? serverDescription.electionId : null;
402-
if (serverDescription.setVersion && electionId) {
403-
if (maxSetVersion && maxElectionId) {
385+
if (serverDescription.electionId != null && serverDescription.setVersion != null) {
386+
if (maxSetVersion != null && maxElectionId != null) {
404387
if (
405-
maxSetVersion > serverDescription.setVersion ||
406-
compareObjectId(maxElectionId, electionId) > 0
388+
compareObjectId(maxElectionId, serverDescription.electionId) === 1 ||
389+
(compareObjectId(maxElectionId, serverDescription.electionId) === 0 &&
390+
maxSetVersion > serverDescription.setVersion)
407391
) {
408392
// this primary is stale, we must remove it
409393
serverDescriptions.set(
@@ -418,6 +402,13 @@ function updateRsFromPrimary(
418402
maxElectionId = serverDescription.electionId;
419403
}
420404

405+
if (
406+
serverDescription.electionId != null &&
407+
(maxElectionId == null || compareObjectId(serverDescription.electionId, maxElectionId) === 1)
408+
) {
409+
maxElectionId = serverDescription.electionId;
410+
}
411+
421412
if (
422413
serverDescription.setVersion != null &&
423414
(maxSetVersion == null || serverDescription.setVersion > maxSetVersion)

‎test/spec/server-discovery-and-monitoring/rs/electionId_setVersion.json‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
{
2+
"description": "ElectionId is considered higher precedence than setVersion",
3+
"uri": "mongodb://a/?replicaSet=rs",
4+
"phases": [
5+
{
6+
"responses": [
7+
[
8+
"a:27017",
9+
{
10+
"ok": 1,
11+
"helloOk": true,
12+
"isWritablePrimary": true,
13+
"hosts": [
14+
"a:27017",
15+
"b:27017"
16+
],
17+
"setName": "rs",
18+
"setVersion": 1,
19+
"electionId": {
20+
"$oid": "000000000000000000000001"
21+
},
22+
"minWireVersion": 0,
23+
"maxWireVersion": 6
24+
}
25+
],
26+
[
27+
"b:27017",
28+
{
29+
"ok": 1,
30+
"helloOk": true,
31+
"isWritablePrimary": true,
32+
"hosts": [
33+
"a:27017",
34+
"b:27017"
35+
],
36+
"setName": "rs",
37+
"setVersion": 2,
38+
"electionId": {
39+
"$oid": "000000000000000000000001"
40+
},
41+
"minWireVersion": 0,
42+
"maxWireVersion": 6
43+
}
44+
],
45+
[
46+
"a:27017",
47+
{
48+
"ok": 1,
49+
"helloOk": true,
50+
"isWritablePrimary": true,
51+
"hosts": [
52+
"a:27017",
53+
"b:27017"
54+
],
55+
"setName": "rs",
56+
"setVersion": 1,
57+
"electionId": {
58+
"$oid": "000000000000000000000002"
59+
},
60+
"minWireVersion": 0,
61+
"maxWireVersion": 6
62+
}
63+
]
64+
],
65+
"outcome": {
66+
"servers": {
67+
"a:27017": {
68+
"type": "RSPrimary",
69+
"setName": "rs",
70+
"setVersion": 1,
71+
"electionId": {
72+
"$oid": "000000000000000000000002"
73+
}
74+
},
75+
"b:27017": {
76+
"type": "Unknown",
77+
"setName": null,
78+
"setVersion": null,
79+
"electionId": null
80+
}
81+
},
82+
"topologyType": "ReplicaSetWithPrimary",
83+
"logicalSessionTimeoutMinutes": null,
84+
"setName": "rs",
85+
"maxSetVersion": 2,
86+
"maxElectionId": {
87+
"$oid": "000000000000000000000002"
88+
}
89+
}
90+
}
91+
]
92+
}

‎test/spec/server-discovery-and-monitoring/rs/electionId_setVersion.yml‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
description: ElectionId is considered higher precedence than setVersion
2+
uri: "mongodb://a/?replicaSet=rs"
3+
phases:
4+
- responses:
5+
- - "a:27017"
6+
- ok: 1
7+
helloOk: true
8+
isWritablePrimary: true
9+
hosts:
10+
- "a:27017"
11+
- "b:27017"
12+
setName: rs
13+
setVersion: 1
14+
electionId:
15+
$oid: "000000000000000000000001"
16+
minWireVersion: 0
17+
maxWireVersion: 6
18+
- - "b:27017"
19+
- ok: 1
20+
helloOk: true
21+
isWritablePrimary: true
22+
hosts:
23+
- "a:27017"
24+
- "b:27017"
25+
setName: rs
26+
setVersion: 2 # Even though "B" reports the newer setVersion, "A" will report the newer electionId which should allow it to remain the primary
27+
electionId:
28+
$oid: "000000000000000000000001"
29+
minWireVersion: 0
30+
maxWireVersion: 6
31+
- - "a:27017"
32+
- ok: 1
33+
helloOk: true
34+
isWritablePrimary: true
35+
hosts:
36+
- "a:27017"
37+
- "b:27017"
38+
setName: rs
39+
setVersion: 1
40+
electionId:
41+
$oid: "000000000000000000000002"
42+
minWireVersion: 0
43+
maxWireVersion: 6
44+
outcome:
45+
servers:
46+
"a:27017":
47+
type: RSPrimary
48+
setName: rs
49+
setVersion: 1
50+
electionId:
51+
$oid: "000000000000000000000002"
52+
"b:27017":
53+
type: Unknown
54+
setName: null
55+
setVersion: null
56+
electionId: null
57+
topologyType: ReplicaSetWithPrimary
58+
logicalSessionTimeoutMinutes: null
59+
setName: rs
60+
maxSetVersion: 2
61+
maxElectionId:
62+
$oid: "000000000000000000000002"

‎test/spec/server-discovery-and-monitoring/rs/use_setversion_without_electionid.json‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -115,22 +115,22 @@
115115
"outcome": {
116116
"servers": {
117117
"a:27017": {
118+
"type": "RSPrimary",
119+
"setName": "rs",
120+
"setVersion": 1
121+
},
122+
"b:27017": {
118123
"type": "Unknown",
119124
"setName": null,
120125
"electionId": null
121-
},
122-
"b:27017": {
123-
"type": "RSPrimary",
124-
"setName": "rs",
125-
"setVersion": 2
126126
}
127127
},
128128
"topologyType": "ReplicaSetWithPrimary",
129129
"logicalSessionTimeoutMinutes": null,
130130
"setName": "rs",
131131
"maxSetVersion": 2,
132132
"maxElectionId": {
133-
"$oid": "000000000000000000000001"
133+
"$oid": "000000000000000000000002"
134134
}
135135
}
136136
}

‎test/spec/server-discovery-and-monitoring/rs/use_setversion_without_electionid.yml‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,21 +97,21 @@ phases: [
9797
outcome: {
9898
servers: {
9999
"a:27017": {
100+
type: "RSPrimary",
101+
setName: "rs",
102+
setVersion: 1
103+
},
104+
"b:27017":{
100105
type: "Unknown",
101106
setName: ,
102107
electionId:
103-
},
104-
"b:27017": {
105-
type: "RSPrimary",
106-
setName: "rs",
107-
setVersion: 2
108108
}
109109
},
110110
topologyType: "ReplicaSetWithPrimary",
111111
logicalSessionTimeoutMinutes: null,
112112
setName: "rs",
113113
maxSetVersion: 2,
114-
maxElectionId: {"$oid": "000000000000000000000001"},
114+
maxElectionId: {"$oid": "000000000000000000000002"},
115115
}
116116
}
117117
]

‎test/unit/assorted/server_discovery_and_monitoring.spec.test.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ describe('Server Discovery and Monitoring (spec)', function () {
6666
};
6767

6868
const specTests = collectTests();
69-
Object.keys(specTests).forEach(specTestName => {
69+
for (const specTestName of Object.keys(specTests)) {
7070
describe(specTestName, () => {
71-
specTests[specTestName].forEach(testData => {
71+
for (const testData of specTests[specTestName]) {
7272
const skip = shouldSkip(testData.description);
7373
const type = skip ? it.skip : it;
7474
type(testData.description, {
@@ -77,9 +77,9 @@ describe('Server Discovery and Monitoring (spec)', function () {
7777
executeSDAMTest(testData, done);
7878
}
7979
});
80-
});
80+
}
8181
});
82-
});
82+
}
8383
});
8484

8585
const OUTCOME_TRANSLATIONS = new Map();

0 commit comments

Comments
 (0)