HELP-58644 Add RequireNew session option #13
Conversation
prestonvasquez
changed the title
|
Testing this branch out locally to confirm it resolves the write conflicts. Will report back shortly EDIT: confirmed that this works! @prestonvasquez I noticed that we're actually currently using a release forked off of 1.12.1 - https://github.com/10gen/baas/blob/5d7540e0a8343c659a86c9417f60624634b12a1b/go.mod#L471 |
prestonvasquez
changed the base branch from
v1.13.1-baas
to
release/v1.12.1-baas
prestonvasquez
changed the base branch from
release/v1.12.1-baas
to
v1.13.1-baas
prestonvasquez
force-pushed
the
HELP-58644
branch
from
75fd21c
to
5cd00b5
Compare
prestonvasquez
changed the base branch from
v1.13.1-baas
to
release/v1.12.1-baas
| @@ -230,7 +234,12 @@ func NewClientSession(pool *Pool, clientID uuid.UUID, opts ...*ClientOptions) (* | |||
| // SetServer will check out a session from the client session pool. | |||
| func (c *Client) SetServer() error { | |||
| var err error | |||
| c.Server, err = c.pool.GetSession() | |||
| if c.RequireNew { | |||
| c.Server, err = newServerSession() | |||
There was a problem hiding this comment.
Can we pass the flag to the pool to alter the behavior there? So users can use the session identically regardless of future changes in the pool.
There was a problem hiding this comment.
Could you elaborate on this?
There was a problem hiding this comment.
Same concern as Matt's underneath. Will join the discussion in his thread.
| @@ -230,7 +234,12 @@ func NewClientSession(pool *Pool, clientID uuid.UUID, opts ...*ClientOptions) (* | |||
| // SetServer will check out a session from the client session pool. | |||
| func (c *Client) SetServer() error { | |||
| var err error | |||
| c.Server, err = c.pool.GetSession() | |||
| if c.RequireNew { | |||
| c.Server, err = newServerSession() | |||
There was a problem hiding this comment.
The session.Server created here will be returned to the pool when EndSession is called. To allow the CheckedOut counter to track this new checked-out session, we should call Pool.createServerSession here instead.
| c.Server, err = newServerSession() | |
| c.Server, err = c.pool.createServerSession() |
Alternatively, if there's concern about pooling these sessions in the Go Driver, we need to find a way to bypass the pool when we end the session.
Will pooling these new sessions in the Go Driver create any issues?
There was a problem hiding this comment.
Good catch, suggest marking the server as dirty when requesting a new server.
prestonvasquez
changed the title
* HELP-58644 Add RequireNew option * HELP-58644 Update field comment * HELP-58644 Clarify RequireNew usage * HELP-58644 Mark single-checkout servers as dirty (cherry picked from commit f6b6041)
* HELP-58644 Add RequireNew option * HELP-58644 Update field comment * HELP-58644 Clarify RequireNew usage * HELP-58644 Mark single-checkout servers as dirty (cherry picked from commit f6b6041)
HELP-58644
Using a snapshot read concern with atClusterTime on a session may result in selecting a server from the server session pool created before atClusterTime, causing a WriteConflict. Due to this potential issue and the fact that it is only possible as of commit b79a739, the Go Driver does not intend to officially support this use case. As a workaround that avoids nondeterministically tearing down the pool (see here for context), the Go Driver team suggests creating a new session every time this scenario arises.