-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(NODE-3697): reduce serverSession allocation #3171
feat(NODE-3697): reduce serverSession allocation #3171
Conversation
a3dc79f
to
ff5b12e
Compare
ff5b12e
to
8f7c72a
Compare
I'm sorry I haven't gotten around to a review yet, but it's on my list to do Monday! |
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.
Much easier to follow the tests now, thanks! I resolved the comments that have been addressed, there are 2 comments with active discussions left, and I noticed a couple more nits here:
Co-authored-by: Daria Pardue <[email protected]>
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.
LGTM assuming CI passes!
Bailey LGTMed given pipeline pass
Description
What is changing?
incrementTransactionNumber
will no longer acquire a serverSession because it won't use the getterWhat is the motivation for this change?
With this change our driver will only acquire a serverSession when the getter is used which is in applySession, used inside the Connection class. This limits the number of serverSession allocated to be bounded by at least the number of concurrent operations running and at best by the max number of connection in our connectionPool.
Double check the following
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>