Skip to content
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.

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

Upgrade MongoDB driver version to v1.14.0 #69

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

kounat
Copy link
Contributor

@kounat kounat commented Apr 3, 2024

Pulling in the latest changes to the MongoDB driver to address CursorNotFound issues when migrated to Optimized Connection Strings in MongoDB Atlas.

Tested locally running docker-compose up and gotestsum.

@@ -1,15 +1,54 @@
module github.com/coinbase/mongobetween

go 1.13
go 1.18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgrading to 1.18 since it's the minimum required version for the latest Mongo driver; doing a go mod tidy afterwards introduced the second require block.

@@ -302,7 +302,7 @@ func (m *Mongo) roundTrip(conn driver.Connection, req []byte, unacknowledged boo
return nil, nil
}

if res, err = conn.ReadWireMessage(m.roundTripCtx, req[:0]); err != nil {
if res, err = conn.ReadWireMessage(m.roundTripCtx); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Latest driver change will now self-create the byte array when reading the wire message: https://github.com/mongodb/mongo-go-driver/blob/v1/x/mongo/driver/topology/connection.go#L471

Comment on lines +28 to +30
func (m *mockServer) RTTMonitor() driver.RTTMonitor {
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating tests to satisfy the latest driver.Server interface

@cb-heimdall
Copy link

Review Error for rubbal90 @ 2024-04-04 02:22:28 UTC
User must have write permissions to review

Copy link
Contributor

@ThatHurleyGuy ThatHurleyGuy left a comment

Choose a reason for hiding this comment

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

@kounat Is there a way for us to build this branch internally and test it in dev/staging before merging this into master?

@cb-heimdall
Copy link

Review Error for rubbalsidhu-cb @ 2024-04-04 02:33:15 UTC
User must have write permissions to review

@kounat
Copy link
Contributor Author

kounat commented Apr 4, 2024

@ThatHurleyGuy yeah, we should be able to pull this branch from the Dockerfile we use to build the sidecar. I'll do that before merging this

@kounat
Copy link
Contributor Author

kounat commented Apr 5, 2024

Tested this internally; it doesn't solve the CursorNotFound issue but I'll still merge this to keep us updated with latest.

@kounat kounat merged commit 1034c5a into coinbase:master Apr 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants