GODRIVER-2539 Remove MinWireVersion < 6 Logic #1084
Conversation
mongo/change_stream.go
Outdated
| @@ -287,7 +287,7 @@ func (cs *ChangeStream) executeOperation(ctx context.Context, resuming bool) err | |||
| // Execute the aggregate, retrying on retryable errors once (1) if retryable reads are enabled and | |||
| // infinitely (-1) if context is a Timeout context. | |||
| var retries int | |||
| if cs.client.retryReads && cs.wireVersion != nil && cs.wireVersion.Max >= 6 { | |||
| if cs.client.retryReads && cs.wireVersion != nil { | |||
There was a problem hiding this comment.
| if cs.client.retryReads && cs.wireVersion != nil { | |
| if cs.client.retryReads { |
The previous check seemed to set retries = 1 if retryable reads were enabled and the wire version was known and greater than or equal to 6. I believe the cs.wireVersion != nil check here used the < 6 wire version logic when the wire version was unknown. I think we can remove it and always operate under the assumption that wire version is greater than 6 given that we no longer support earlier wire versions.
There was a problem hiding this comment.
@benjirewis I don't quite follow this. So cs.wireVersion would only be nil if the wire version was < 6?
mongo/change_stream.go
Outdated
| cs.wireVersion = conn.Description().WireVersion | ||
| if cs.wireVersion == nil || cs.wireVersion.Max < 6 { | ||
| if cs.wireVersion == nil { |
There was a problem hiding this comment.
See my previous comment. I think we can remove this entire wire version check.
x/mongo/driver/operation.go
Outdated
| @@ -830,7 +812,7 @@ func (op Operation) retryable(desc description.Server) bool { | |||
| return true | |||
| } | |||
| if retryWritesSupported(desc) && | |||
| desc.WireVersion != nil && desc.WireVersion.Max >= 6 && | |||
| desc.WireVersion != nil && | |||
There was a problem hiding this comment.
See my previous comment. I think we can remove the desc.WireVersion != nil piece of this conditional.
x/mongo/driver/operation.go
Outdated
| @@ -839,7 +821,7 @@ func (op Operation) retryable(desc description.Server) bool { | |||
| if op.Client != nil && (op.Client.Committing || op.Client.Aborting) { | |||
| return true | |||
| } | |||
| if desc.WireVersion != nil && desc.WireVersion.Max >= 6 && | |||
| if desc.WireVersion != nil && | |||
There was a problem hiding this comment.
See my previous comment. I think we can remove the desc.WireVersion != nil piece of this conditional.
|
Now that we don't need to check wire version for resuming change streams, I wonder if we can simplify |
| @@ -325,12 +325,6 @@ AggregateExecuteLoop: | |||
| } | |||
| defer conn.Close() | |||
|
|
|||
| // If wire version is now < 6, do not retry. | |||
| cs.wireVersion = conn.Description().WireVersion | |||
There was a problem hiding this comment.
Do we need to update the wireVersion?
There was a problem hiding this comment.
Ah yes, I think we should. Nice catch, thank you!
GODRIVER-2539
#1063 bumps the minimum wire version from 2 to 6. The scope of GODRIVER-2539 is to remove logic dependent on MinWireVersion < 6 in "x/mongo/driver/operation.go" and "mongo/change_stream.go".
Deprecating "mongo/integration/operation_legacy_test.go"
In an internal discussion about this ticket, the following came up:
The tests in this file use the function
validateQueryWireMessagewhich iswiremessage.ReadQuery*focused. Most of these "ReadQuery" functions do not have "ReadMsg" analogue, the only one beingReadQueryFlags.ReadMsgFlagsalready has test coverage here.Deprecating "x/mongo/driver/operation_legacy.go"
The functionality of this file is exclusively used in the two legacy branches removed from the "driver.Operation#Execute" function and was untested outside of "mongo/integration/operation_legacy_test.go".