GODRIVER-2828 Use topology version from Server instead of Connection in ProcessError. #1252

matthewdale · 2023-05-09T05:17:16Z

Summary

Use the latest topology version from either the server or connection when processing errors in ProcessError.
Refactor the ProcessError test to express the expected initial and resulting state changes more clearly.
Add test cases for when the connection topology version is newer than the server topology version and when the topology process ID has changed.

Background & Motivation

While processing errors, the SDAM error handling function ProcessError attempts to compare the topology version on the error to the current topology version known by the Go driver, ignoring errors where the topology version is not newer than the known topology version. However, the Go driver currently uses the connection's topology version as the "known" topology version, which is never updated after the connection is created. As a result, ProcessError will act on errors from older connections when it should ignore them.

qingyang-hu

👍

prestonvasquez · 2023-05-22T21:52:00Z

x/mongo/driver/topology/server.go

-		// ignore stale error
-		if desc.TopologyVersion.CompareToIncoming(cerr.TopologyVersion) >= 0 {
+		// Ignore errors that came from when the database was on a previous topology version.
+		if topologyVersion.CompareToIncoming(cerr.TopologyVersion) >= 0 {


In the case where the max{ServerTopology, ConnectionTopology} = ErrorTopology , why do we consider this a "previous topology version"? If this is correct behavior, can we add a comment explaining it?

// In the case where the max{serverDesc.TopologyVersion, connDesc.TopologyVersion} == cerr.TopologyVersion , // we consider cerr.Topology version a "previous topology version" because ...

Good point, the current comment actually describes slightly different behavior because the logic also ignores errors that came from the current topology version.

I believe the reason we only act on errors where the error topology version is strictly greater than the local topology version is that we only want to process the first instance of that error for a given topology version, and ignore the rest.

For example consider the following sequence of events:

Step Action Driver TopologyVersion DB TopologyVersion

1 Initial state 1 1

2 DB elects new primary 1 2

3 Driver starts 4 concurrent writes to secondary, secondary returns "not writable primary" 1 2

4 Driver processes first "not writable primary" error with TopologyVersion=2, marks server as Kind=Unknown, updates its TopologyVersion 2 2

5 Driver processes 3 more "not writable primary" errors with TopologyVersion=2, ignores them 2 2

Notice that the driver only processes the first "not writable primary" error, marking the server role as "unknown" until it is updated by the monitoring routine. Subsequent errors are ignored based on topology version, which prevents the driver from re-processing errors for the same topology change event.

Keep in mind that we only check topology version for "node is recovering" or "not primary" error codes, which we expect to increment topology version on the database. The first time the driver sees one of those errors, it knows the database topology has changed and needs to re-discover the topology. The driver ignores topology version for all other kinds of errors.

For additional reference, the topology version comparison logic comes directly from the error handling pseudocode in the SDAM spec.

I'll update the comment with a summary of the above information to increase clarity.

Oh, I see. This great explanation! Thank you

prestonvasquez · 2023-05-22T21:55:38Z

x/mongo/driver/topology/server.go

-		// ignore stale error
-		if desc.TopologyVersion.CompareToIncoming(wcerr.TopologyVersion) >= 0 {
+		// Ignore errors that came from when the database was on a previous topology version.
+		if topologyVersion.CompareToIncoming(wcerr.TopologyVersion) >= 0 {


In the case where the max{ServerTopology, ConnectionTopology} = ErrorTopology , why do we consider this a "previous topology version"? If this is correct behavior, can we add a comment explaining it?

// In the case where the max{serverDesc.TopologyVersion, connDesc.TopologyVersion} == wcerr.TopologyVersion , // we consider cerr.Topology version a "previous topology version" because ...

See this comment for a detailed answer.

prestonvasquez · 2023-05-22T22:01:12Z

x/mongo/driver/topology/server.go

@@ -434,9 +467,9 @@ func (s *Server) ProcessError(err error, conn driver.Connection) driver.ProcessE

 		res := driver.ServerMarkedUnknown
 		// If the node is shutting down or is older than 4.2, we synchronously clear the pool
-		if wcerr.NodeIsShuttingDown() || desc.WireVersion == nil || desc.WireVersion.Max < 8 {
+		if wcerr.NodeIsShuttingDown() || wireVersion == nil || wireVersion.Max < 8 {


What are your thoughts on creating a constant for "8"? Even though comparing wireVersions to their literal number is commonplace in the driver, it is a magic number which makes it sort of hard to interpret.

const wireVersionGEQ42 = 8

Yeah, using a named constant would help with readability. Will update.

x/mongo/driver/topology/server_test.go

prestonvasquez · 2023-05-22T22:17:21Z

x/mongo/driver/topology/server_test.go

 }

-func newProcessErrorTestConn(tv *description.TopologyVersion) *processErrorTestConn {
+func newProcessErrorTestConn(wireVersion *description.VersionRange, stale bool) *processErrorTestConn {


Can we put this and "newServerDescription" in the namespace? i.e. either both as package test functions, or both as closure functions in the test function?

The processErrorTestConn can't be defined in the ProcessError test function because it needs to define methods to satisfy the driver.Connection interface. However, I can move newServerDescription to the package scope.

prestonvasquez · 2023-05-22T22:18:46Z

x/mongo/driver/topology/server.go

@@ -415,16 +448,16 @@ func (s *Server) ProcessError(err error, conn driver.Connection) driver.ProcessE

 		res := driver.ServerMarkedUnknown
 		// If the node is shutting down or is older than 4.2, we synchronously clear the pool
-		if cerr.NodeIsShuttingDown() || desc.WireVersion == nil || desc.WireVersion.Max < 8 {
+		if cerr.NodeIsShuttingDown() || wireVersion == nil || wireVersion.Max < 8 {


What are your thoughts on creating a constant for "8"? Even though comparing wireVersions to their literal number is commonplace in the driver, it is a magic number which makes it sort of hard to interpret.

const wireVersionGEQ42 = 8

Yeah, using a named constant would help with readability. Will update.

x/mongo/driver/topology/server_test.go

prestonvasquez · 2023-05-22T22:22:47Z

x/mongo/driver/topology/server_test.go

+			name:             "stale not primary error",
+			startDescription: newServerDescription(description.RSPrimary, processID, 1, nil),
+			inputErr: driver.Error{
+				Code: 10107, // NotPrimary


Optional: 10107 is "NotWritablePrimary", see here.

Good catch. That error is documented as "NotPrimary" in many places in the Go driver code. I'll correct it in these tests and then open a subsequent PR to correct it elsewhere in the Go driver.

…in ProcessError.

Co-authored-by: Preston Vasquez <prestonvasquez@icloud.com>

…in ProcessError. (#1252) Co-authored-by: Preston Vasquez <prestonvasquez@icloud.com>

matthewdale force-pushed the godriver2828-sdam-top-version branch 4 times, most recently from 3e86d65 to 0c97b45 Compare May 16, 2023 17:59

matthewdale marked this pull request as ready for review May 16, 2023 18:10

matthewdale requested review from prestonvasquez and qingyang-hu May 16, 2023 18:22

qingyang-hu approved these changes May 18, 2023

View reviewed changes

prestonvasquez reviewed May 22, 2023

View reviewed changes

matthewdale and others added 3 commits May 22, 2023 18:09

GODRIVER-2828 Use topology version from Server instead of Connection …

935cb25

…in ProcessError.

Apply suggestions from code review

6586cce

Co-authored-by: Preston Vasquez <prestonvasquez@icloud.com>

Code review changes.

af7f854

matthewdale force-pushed the godriver2828-sdam-top-version branch from dda23d3 to af7f854 Compare May 24, 2023 19:07

matthewdale requested a review from prestonvasquez May 24, 2023 20:33

prestonvasquez approved these changes May 25, 2023

View reviewed changes

matthewdale merged commit 71edba3 into mongodb:master Jun 5, 2023

matthewdale added a commit that referenced this pull request Jun 5, 2023

GODRIVER-2828 Use topology version from Server instead of Connection …

d797822

…in ProcessError. (#1252) Co-authored-by: Preston Vasquez <prestonvasquez@icloud.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-2828 Use topology version from Server instead of Connection in ProcessError. #1252

GODRIVER-2828 Use topology version from Server instead of Connection in ProcessError. #1252

matthewdale commented May 9, 2023 •

edited

Loading

qingyang-hu left a comment

prestonvasquez May 22, 2023

matthewdale May 23, 2023 •

edited

Loading

prestonvasquez May 24, 2023

prestonvasquez May 22, 2023

matthewdale May 24, 2023

prestonvasquez May 22, 2023

matthewdale May 22, 2023

prestonvasquez May 22, 2023

matthewdale May 23, 2023

prestonvasquez May 22, 2023

matthewdale May 22, 2023

prestonvasquez May 22, 2023

matthewdale May 22, 2023

Step	Action	Driver TopologyVersion	DB TopologyVersion
1	Initial state	1	1
2	DB elects new primary	1	2
3	Driver starts 4 concurrent writes to secondary, secondary returns "not writable primary"	1	2
4	Driver processes first "not writable primary" error with TopologyVersion=2, marks server as Kind=Unknown, updates its TopologyVersion	2	2
5	Driver processes 3 more "not writable primary" errors with TopologyVersion=2, ignores them	2	2

GODRIVER-2828 Use topology version from Server instead of Connection in ProcessError. #1252

GODRIVER-2828 Use topology version from Server instead of Connection in ProcessError. #1252

Conversation

matthewdale commented May 9, 2023 • edited Loading

Summary

Background & Motivation

qingyang-hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdale May 23, 2023 • edited 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

matthewdale commented May 9, 2023 •

edited

Loading

matthewdale May 23, 2023 •

edited

Loading