Skip to content

DRIVERS-2972: add message requirement to ServerDescription.error #1729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Feb 4, 2025
Merged

DRIVERS-2972: add message requirement to ServerDescription.error #1729

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ea5698d
add requirement to error field
W-A-James Nov 8, 2024
6b75475
update workding
W-A-James Nov 12, 2024
2668eb9
update to change server description in correct place
W-A-James Nov 18, 2024
889e3dd
update wording
W-A-James Nov 25, 2024
75ce4dd
Update algorithm to allow for more granular setting of error messages
W-A-James Nov 27, 2024
bc971f2
update table
W-A-James Nov 27, 2024
5d121b1
update table
W-A-James Dec 2, 2024
21187a9
update tests
W-A-James Dec 2, 2024
b7adc0b
regen json test files
W-A-James Dec 2, 2024
b6dfeee
remove unnecessary file
W-A-James Dec 2, 2024
07976f0
update comment
W-A-James Jan 13, 2025
cd84c0e
update new_primary test
W-A-James Jan 13, 2025
a2bd209
Revert "update comment"
W-A-James Jan 13, 2025
becaa6b
update table and generate test file
W-A-James Jan 13, 2025
47777df
update error message
W-A-James Jan 17, 2025
483db4e
update changelog
W-A-James Jan 22, 2025
8bf359e
simplify changes
W-A-James Jan 23, 2025
70532c7
update prose
W-A-James Jan 24, 2025
9d51f24
move sentence
W-A-James Jan 24, 2025
cdb27c9
update table
W-A-James Jan 24, 2025
e97908e
Merge branch 'master' into DRIVERS-2972
W-A-James Jan 24, 2025
4b8516d
fix test assertions
W-A-James Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 31 additions & 15 deletions source/server-discovery-and-monitoring/server-discovery-and-monitoring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ Fields:
field in the server's hello or legacy hello response, in the case that the server reports an address different from
the address the client uses.

- (=) `error`: information about the last error related to this server. Default null.
- (=) `error`: information about the last error related to this server. Default null. MUST contain or be able to produce
a string describing the error.

- `roundTripTime`: the duration of the hello or legacy hello call. Default null.

Expand Down Expand Up @@ -485,7 +486,13 @@ removed once the primary is checked.
#### error

If the client experiences any error when checking a server, it stores error information in the ServerDescription's error
field.
field. The message contained in this field MUST contain the substrings detailed in the table below when the
ServerDescription is changed to Unknown in the circumstances outlined.

| circumstance | error substring |
| ---------------------------------------------------------- | -------------------------------------------------------------- |
| RSPrimary with a stale electionId/setVersion is discovered | `'primary marked stale due to electionId/setVersion mismatch'` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this comment #1729 (comment) I had suggested adding new and old election tuples to the error message. Did you decide not to do that? Or could you add an example of the suggested error message here?

Copy link
Contributor Author

@W-A-James W-A-James Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, just missed that. Here's what the message looks like in the Node driver currently (implemented before we finalized this):

MongoStalePrimaryError: primary marked stale due to electionId/setVersion mismatch: server setVersion: 1, server electionId: 000000000000000000000001, topology setVersion: 1, topology electionId: 000000000000000000000002

After implementing this it could look like

MongoStalePrimaryError: primary marked stale due to electionId/setVersion mismatch: stale electionId/setVersion: (000000000000000000000001,1), new electionId/setVersion: (000000000000000000000002, 1)

| New primary is elected/discovered | `'primary marked stale due to discovery of newer primary'` |

#### roundTripTime

Expand Down Expand Up @@ -871,7 +878,8 @@ if serverDescription.maxWireVersion >= 17: # MongoDB 6.0+
topologyDescription.maxSetVersion = serverDescription.setVersion
else:
# Stale primary.
# replace serverDescription with a default ServerDescription of type "Unknown"
# replace serverDescription with a default ServerDescription of type "Unknown" and an error
# field with a message containing the substring "primary marked stale due to electionId/setVersion mismatch"
checkIfHasPrimary()
return
else:
Expand All @@ -889,7 +897,8 @@ else:
)
):
# Stale primary.
# replace serverDescription with a default ServerDescription of type "Unknown"
# replace serverDescription with a default ServerDescription of type "Unknown" and an error
# field with a message containing the substring "primary marked stale due to electionId/setVersion mismatch"
checkIfHasPrimary()
return

Expand All @@ -906,7 +915,8 @@ for each server in topologyDescription.servers:
if server.address != serverDescription.address:
if server.type is RSPrimary:
# See note below about invalidating an old primary.
replace the server with a default ServerDescription of type "Unknown"
# replace the server with a default ServerDescription of type "Unknown"
# and an error field with a message containing the subsring "primary marked stale due to discovery of newer primary"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit off to have an if-statement code block with only comments. How about:

        if server.type is RSPrimary:
            # See note below about invalidating an old primary.
            # The error field MUST include the substring "primary marked stale due to discovery of newer primary"
            replace the server with a default ServerDescription of type "Unknown"


for each address in serverDescription's "hosts", "passives", and "arbiters":
if address is not in topologyDescription.servers:
Expand All @@ -921,8 +931,11 @@ checkIfHasPrimary()
```

A note on invalidating the old primary: when a new primary is discovered, the client finds the previous primary (there
should be none or one) and replaces its description with a default ServerDescription of type "Unknown." A multi-threaded
client MUST [request an immediate check](server-monitoring.md#requesting-an-immediate-check) for that server as soon as
should be none or one) and replaces its description with a default ServerDescription of type "Unknown". Additionally,
the `error` field of the new `ServerDescription` object MUST include a descriptive error explaining that it was
invalidated because the primary was determined to be stale. Drivers MAY additionally specify whether this was due to an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph is "a note on invalidating the old primary" so I don't think it applies to all the "electionId/setVersion mismatch" case and this sentence should be moved:

Drivers MAY additionally specify whether this was due to an electionId or setVersion mismatch as described in the ServerDescripion.error section.

It should be moved to the paragraph below that starts with "If the server is primary with an obsolete electionId or setVersion,"

electionId or setVersion mismatch as described in the [ServerDescripion.error section](#error). A multi-threaded client
MUST [request an immediate check](server-monitoring.md#requesting-an-immediate-check) for that server as soon as
possible.

If the old primary server version is 4.0 or earlier, the client MUST clear its connection pool for the old primary, too:
Expand Down Expand Up @@ -1921,14 +1934,6 @@ oversaw the specification process.

## Changelog

- 2024-11-04: Make the description of `TopologyDescription.servers` consistent with the spec tests.

- 2024-08-16: Updated host b wire versions in `too_new` and `too_old` tests

- 2024-08-09: Updated wire versions in tests to 4.0+.

- 2024-05-08: Migrated from reStructuredText to Markdown.

- 2015-12-17: Require clients to compare (setVersion, electionId) tuples.

- 2015-10-09: Specify electionID comparison method.
Expand Down Expand Up @@ -2010,6 +2015,17 @@ oversaw the specification process.

- 2024-01-17: Add section on expected client close behaviour

- 2024-05-08: Migrated from reStructuredText to Markdown.

- 2024-08-09: Updated wire versions in tests to 4.0+.

- 2024-08-16: Updated host b wire versions in `too_new` and `too_old` tests

- 2024-11-04: Make the description of `TopologyDescription.servers` consistent with the spec tests.

- 2025-01-22: Add error messages when a new primary is elected or a primary with a stale electionID or setVersion is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

electionID -> electionId

discovered.

______________________________________________________________________

[^1]: "localThresholdMS" was called "secondaryAcceptableLatencyMS" in the Read Preferences Spec, before it was superseded
Expand Down
2 changes: 2 additions & 0 deletions source/server-discovery-and-monitoring/tests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ following keys:

- type: A ServerType name, like "RSSecondary". See [ServerType](../server-discovery-and-monitoring.md#servertype) for
details pertaining to async and multi-threaded drivers.
- error: An optional object with a with a string field containing a string that must be a substring of the message on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An optional object with a with a string field containing a string that must be...
->
An optional string that must be...

the `ServerDescription.error` object
- setName: A string with the expected replica set name, or null.
- setVersion: absent or an integer.
- electionId: absent, null, or an ObjectId.
Expand Down
3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/new_primary.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/new_primary.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ phases: [
"a:27017": {

type: "Unknown",
setName:
setName:,
error: "primary marked stale due to discovery of newer primary"
},

"b:27017": {
Expand Down
6 changes: 4 additions & 2 deletions source/server-discovery-and-monitoring/tests/rs/new_primary_new_electionid.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions source/server-discovery-and-monitoring/tests/rs/new_primary_new_electionid.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId: ,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down Expand Up @@ -100,7 +101,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
6 changes: 4 additions & 2 deletions source/server-discovery-and-monitoring/tests/rs/new_primary_new_setversion.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions source/server-discovery-and-monitoring/tests/rs/new_primary_new_setversion.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down Expand Up @@ -100,7 +101,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/primary_disconnect_electionid.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/primary_disconnect_electionid.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/primary_disconnect_setversion.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/primary_disconnect_setversion.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
3 changes: 2 additions & 1 deletion ...rver-discovery-and-monitoring/tests/rs/setversion_greaterthan_max_without_electionid.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion ...erver-discovery-and-monitoring/tests/rs/setversion_greaterthan_max_without_electionid.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/setversion_without_electionid-pre-6.0.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion source/server-discovery-and-monitoring/tests/rs/setversion_without_electionid-pre-6.0.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
6 changes: 4 additions & 2 deletions ...e/server-discovery-and-monitoring/tests/rs/use_setversion_without_electionid-pre-6.0.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions ...ce/server-discovery-and-monitoring/tests/rs/use_setversion_without_electionid-pre-6.0.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down Expand Up @@ -99,7 +100,8 @@ phases: [
"a:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
},
"b:27017": {
type: "RSPrimary",
Expand Down
6 changes: 4 additions & 2 deletions source/server-discovery-and-monitoring/tests/rs/use_setversion_without_electionid.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions source/server-discovery-and-monitoring/tests/rs/use_setversion_without_electionid.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ phases: [
"b:27017": {
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
}
},
topologyType: "ReplicaSetWithPrimary",
Expand Down Expand Up @@ -106,7 +107,8 @@ phases: [
"b:27017":{
type: "Unknown",
setName: ,
electionId:
electionId:,
error: "primary marked stale due to electionId/setVersion mismatch"
}
},
topologyType: "ReplicaSetWithPrimary",
Expand Down