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

W-A-James · 2024-11-12T15:44:29Z

Please complete the following before merging:

Update changelog.
Test changes in at least one language driver.
Test these changes against all server versions and topologies (including standalone, replica set, sharded
clusters, and serverless).

ShaneHarvey · 2024-11-22T18:44:52Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

+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
+electionId or setVersion mismatch. A multi-threaded client MUST
+[request an immediate check](server-monitoring.md#requesting-an-immediate-check) for that server as soon as possible.


Should we expand the scope of this ticket to include more info in other cases where we reset a server to "unknown" besides stale primary?

yeah I think that makes sense to do here. Seems like the only other place we do that is in the handleError function, so I'll take a look at that.

So there are two places we do this in our handleError logic, but in both cases, the handleError function takes in an error as a parameter. Do we still want to be testing against the errors here?

ShaneHarvey · 2024-11-22T18:45:18Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

+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
+electionId or setVersion mismatch. A multi-threaded client MUST


Let's get more specific about what the error should look like so that we can add tests.

I purposely left this a little more open-ended to give drivers leeway to use the Error/Exception API most native to their language. I can say that more explicitly in the ServerDescription.error section.

Updated to cross-reference the ServerDescription.error section

ShaneHarvey · 2024-11-27T18:54:00Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

- (=) `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. The name of the field containing the string describing the error SHOULD be what is
+    most idiomatic for each driver.


What was the motivation for adding "The name of the field containing the string describing the error SHOULD be what is most idiomatic for each driver."?

I was thinking that the error field would be an object representing an error and since different languages have different ways of doing that, I think it's better to defer to driver engineers on what is the best way to communicate that to users. In Node we'd likely do that with a new MongoError subclass (which itself subclasses the native Node Error type) that uses a message field to hold human readable information about the error. We currently have it typed as a generic MongoError

I see. I think we can remove that sentence since it generally always true of any spec prescribed api.

ShaneHarvey · 2024-11-27T18:57:02Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

+| RSPrimary with a stale electionId is discovered                      | `'primary marked stale due to electionId mismatch'`      |
+| RSPrimary with a stale setVersion is discovered                      | `'primary marked stale due to setVersion mismatch'`      |


Can we combine these two cases and include both the stale and max election tuples?

Yeah, that's fair

ShaneHarvey · 2024-11-27T18:59:01Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

+| -------------------------------------------------------------------- | -------------------------------------------------------- |
+| RSPrimary with a stale electionId is discovered                      | `'primary marked stale due to electionId mismatch'`      |
+| RSPrimary with a stale setVersion is discovered                      | `'primary marked stale due to setVersion mismatch'`      |
+| A more current RSPrimary is discovered alongside an existing primary | `'primary marked stale due to discovery of new primary'` |


Should we combine this case as well? IIUC the case is the same as the above but just from the other server's point of view.

W-A-James · 2024-12-03T18:46:57Z

Node POC: mongodb/node-mongodb-native#4340

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

This reverts commit 07976f0.

ShaneHarvey · 2025-01-15T23:33:36Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

-    else:
-        # Stale primary.
-        # replace serverDescription with a default ServerDescription of type "Unknown"
+    if serverDescription.electionId < topologyDescription.maxElectionId:


I see we the pseudocode is a bit more verbose now that the logic needs to distinguish between the electionId vs setVersion stale cases. What do you think about one error message that include the two tuples:

"primary marked stale due to electionId/setVersion mismatch, <stale tuple> is stale compared to <max tuple>"

That way we don't need to refactor this code and more importantly drivers don't need to refactor their comparison logic.

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

ShaneHarvey · 2025-01-23T23:34:07Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

@@ -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"


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"

ShaneHarvey · 2025-01-23T23:40:33Z

source/server-discovery-and-monitoring/tests/README.md

@@ -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


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

ShaneHarvey · 2025-01-23T23:42:17Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

+
+- 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


electionID -> electionId

ShaneHarvey · 2025-01-23T23:46:45Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

-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


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,"

ShaneHarvey · 2025-01-23T23:50:40Z

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md

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


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?

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)

ShaneHarvey

I just implemented this in Python and found that some of these tests are expecting an incorrect error message:

$ pytest -s -v -k 'new_primary or electionid or setversion' -- test/test_discovery_and_monitoring.py
                                                                               
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_electionId_precedence_setVersion PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_equal_electionids PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_new_primary PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_new_primary_new_electionid FAILED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_new_primary_new_setversion FAILED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_new_primary_wrong_set_name PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_primary_disconnect_electionid FAILED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_primary_disconnect_setversion FAILED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_setversion_equal_max_without_electionid PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_setversion_greaterthan_max_without_electionid FAILED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_setversion_without_electionid PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_setversion_without_electionid-pre-6.0 FAILED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_use_setversion_without_electionid PASSED
test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_use_setversion_without_electionid-pre-6.0 FAILED

FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_new_primary_new_electionid - AssertionError: phase: 1: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_new_primary_new_setversion - AssertionError: phase: 1: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_primary_disconnect_electionid - AssertionError: phase: 0: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_primary_disconnect_setversion - AssertionError: phase: 0: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_setversion_greaterthan_max_without_electionid - AssertionError: phase: 1: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_setversion_without_electionid-pre-6.0 - AssertionError: phase: 1: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
FAILED test/test_discovery_and_monitoring.py::TestAllScenarios::test_rs_use_setversion_without_electionid-pre-6.0 - AssertionError: phase: 1: 'primary marked stale due to electionId/setVersion mismatch' not found in 'primary marked stale due to discovery of newer...
===================================================== 7 failed, 7 passed, 219 deselected in 0.48s ======================================================

W-A-James · 2025-02-04T21:05:26Z

@ShaneHarvey updated Node POC and tests mongodb/node-mongodb-native#4397

ShaneHarvey

LGTM, tests pass in Python.

W-A-James added 3 commits November 8, 2024 15:38

add requirement to error field

ea5698d

update workding

6b75475

update to change server description in correct place

2668eb9

ShaneHarvey requested changes Nov 22, 2024

View reviewed changes

W-A-James added 3 commits November 25, 2024 15:17

update wording

889e3dd

Update algorithm to allow for more granular setting of error messages

75ce4dd

update table

bc971f2

ShaneHarvey reviewed Nov 27, 2024

View reviewed changes

W-A-James added 4 commits December 2, 2024 15:39

update table

5d121b1

update tests

21187a9

regen json test files

b7adc0b

remove unnecessary file

b6dfeee

W-A-James requested a review from ShaneHarvey December 3, 2024 18:44

W-A-James changed the title ~~add requirement to error field~~ DRIVERS-2972: add message requirement to ServerDescription.error Dec 3, 2024

W-A-James marked this pull request as ready for review December 3, 2024 18:57

W-A-James requested a review from a team as a code owner December 3, 2024 18:57

ShaneHarvey requested changes Jan 10, 2025

View reviewed changes

source/server-discovery-and-monitoring/server-discovery-and-monitoring.md Outdated Show resolved Hide resolved

W-A-James added 4 commits January 13, 2025 14:36

update comment

07976f0

update new_primary test

cd84c0e

Revert "update comment"

a2bd209

This reverts commit 07976f0.

update table and generate test file

becaa6b

W-A-James requested a review from ShaneHarvey January 15, 2025 21:53

ShaneHarvey requested changes Jan 15, 2025

View reviewed changes

W-A-James added 3 commits January 17, 2025 13:19

update error message

47777df

update changelog

483db4e

simplify changes

8bf359e

W-A-James requested a review from ShaneHarvey January 23, 2025 19:33

ShaneHarvey requested changes Jan 23, 2025

View reviewed changes

update prose

70532c7

W-A-James added 3 commits January 24, 2025 16:10

move sentence

9d51f24

update table

cdb27c9

Merge branch 'master' into DRIVERS-2972

e97908e

W-A-James requested a review from ShaneHarvey January 24, 2025 22:18

ShaneHarvey requested changes Jan 31, 2025

View reviewed changes

ShaneHarvey mentioned this pull request Jan 31, 2025

PYTHON-4960 More informative error message for stale primary mongodb/mongo-python-driver#2115

Draft

fix test assertions

4b8516d

W-A-James requested a review from ShaneHarvey February 4, 2025 21:05

ShaneHarvey approved these changes Feb 4, 2025

View reviewed changes

W-A-James merged commit a6897dc into mongodb:master Feb 4, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

W-A-James commented Nov 12, 2024 •

edited by ShaneHarvey

Loading

ShaneHarvey Nov 22, 2024

W-A-James Nov 27, 2024

W-A-James Dec 2, 2024

ShaneHarvey Nov 22, 2024

W-A-James Nov 27, 2024

W-A-James Dec 3, 2024

ShaneHarvey Nov 27, 2024

W-A-James Nov 27, 2024

ShaneHarvey Nov 27, 2024

ShaneHarvey Nov 27, 2024

W-A-James Nov 27, 2024

ShaneHarvey Nov 27, 2024

W-A-James commented Dec 3, 2024

ShaneHarvey Jan 15, 2025

ShaneHarvey Jan 23, 2025

ShaneHarvey Jan 23, 2025

ShaneHarvey Jan 23, 2025

ShaneHarvey Jan 23, 2025

ShaneHarvey Jan 23, 2025

W-A-James Jan 24, 2025 •

edited

Loading

ShaneHarvey left a comment

W-A-James commented Feb 4, 2025

ShaneHarvey left a comment

		\| RSPrimary with a stale electionId is discovered \| `'primary marked stale due to electionId mismatch'` \|
		\| RSPrimary with a stale setVersion is discovered \| `'primary marked stale due to setVersion mismatch'` \|


		- 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

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

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

Conversation

W-A-James commented Nov 12, 2024 • edited by ShaneHarvey Loading

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

W-A-James commented Dec 3, 2024

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

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

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

W-A-James commented Feb 4, 2025

ShaneHarvey left a comment

Choose a reason for hiding this comment

W-A-James commented Nov 12, 2024 •

edited by ShaneHarvey

Loading

W-A-James Jan 24, 2025 •

edited

Loading