-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-3612 Add an internal-only NewSessionWithLSID API (v2) #2177
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
GODRIVER-3612 Add an internal-only NewSessionWithLSID API (v2) #2177
Conversation
API Change ReportNo changes found! |
🧪 Performance ResultsCommit SHA: 03adad0The following benchmark tests for version 68b22835a327320007f01f24 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
bc7a505
to
9eb374b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds an internal-only API mongo.NewSessionWithID
that creates a *Session
for a specified Client
and session ID document. This API is intended for internal use cases where specific session IDs need to be used rather than letting the driver generate them automatically.
- Introduces
NewSessionWithID
function that constructs sessions with custom session ID documents - Implements build tag protection (
//go:build mongointernal
) to restrict usage to internal builds only - Updates CI configuration to enable the mongointernal build tag in tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
mongo/mongointernal.go | Implements the new NewSessionWithID API with proper documentation and build tag restrictions |
internal/integration/mongointernal_test.go | Adds comprehensive tests covering both successful usage and expected panic behavior |
.evergreen/config.yml | Updates CI build tags to include mongointernal for proper test execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about users using session.Client methods that involve a pool. Or worse, maintainers adding a new method to session.Client that requires accessing the pool and forgetting to document that behavior for NewSessionWithID
.
I've created GODRIVER-3649 to follow up on this in 3.0.
mongo/mongointernal.go
Outdated
// | ||
// NewSessionWithID is intended only for internal use and may be changed or | ||
// removed at any time. | ||
func NewSessionWithID(client *Client, sessionID bson.Raw) *Session { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Suggest renaming this function to NewSessionWithLSID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
//go:build mongointernal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] Should we name this mongointernal_lsid to keep it separate from any future code we may want to gate behind a mongointernal build tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned too many build tags will become a maintenance headache, both for us and for the internal teams who use the Go Driver. I'm assuming that an app that uses one internal-only API may need to use many internal-only APIs, and keeping track of a list of build tags may become onerous.
Is there a scenario you're thinking of where it's better to have more granular build tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a scenario you're thinking of where it's better to have more granular build tags?
No, and the more I think about it it seems unlikely that we will need to use mongointernal
often.
mt.Parallel() | ||
|
||
sessionID := bson.Raw(bsoncore.NewDocumentBuilder(). | ||
AppendBinary("id", 4, []byte{}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Suggest using a valid 16-byte UUID to ensure the panic is due to the session being unpooled and not an invalid LSID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
mongo/mongointernal.go
Outdated
// subtype 4). | ||
// | ||
// Sessions returned by NewSessionWithID are never added to the driver's session | ||
// pool. Calling EndSession on a Session returned by NewSessionWithID will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] We should include SetServer
as API that will panic. I'm not sure why anyone would use it and AFAIK it's only use case happens for implicit sessions, but it's still possible:
sess.Client().SetSrerver() // will panic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added note that ClientSession().SetServer()
will panic and a test to confirm it does.
The base branch was changed.
I just realized this needs to target |
GODRIVER-3612
Summary
Add an internal-only API
mongo.NewSessionWithLSID
that returns a*Session
for the specifiedClient
and session ID document.Background & Motivation