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

feat(NODE-2993): implement maxConnecting #3255

Merged
merged 14 commits into from May 23, 2022

Conversation

dariakp
Copy link
Contributor

@dariakp dariakp commented May 19, 2022

Description

NODE-2993

What is changing?

  • Connection pool can now attempt to check out up to maxConnecting number of connections in parallel (previously it would only establish the requested connections one at a time)
  • ensureMinPoolSize will now only attempt to create one connection at a time so as not to block checkout requests from obtaining connections as quickly as possible (since ensureMinPoolSize always adds the connections to the available pool first, a checkout request would otherwise have to wait to retrieve it from the pool instead of obtaining it immediately; this nonblocking behavior is relied upon by the "threads blocked by maxConnecting check out minPoolSize connections" spec test)
  • The ConnectionCreatedEvent is now fired before actually establishing the connection, as required by the spec (the maxConnecting tests rely on this to differentiate between pending and established connections)
  • There was a bug in the cmap runner where the operations weren't actually queued using the main thread but rather in parallel across threads, this PR includes a fix for that
  • Note: the new waitQueueTimeoutMS test is skipped because it is not applicable given our waitQueueTimeoutMS implementation (see the corresponding test yaml file for the relevant note)
Is there new documentation needed for these changes?

No

What is the motivation for this change?

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
@dariakp dariakp added wip and removed wip labels May 19, 2022
@dariakp dariakp marked this pull request as ready for review May 19, 2022 21:51
nbbeeken
nbbeeken previously approved these changes May 19, 2022
@nbbeeken nbbeeken added the Team Review Needs review from team label May 19, 2022
@dariakp dariakp force-pushed the NODE-2993/implement-max-connecting branch from 92d6ec8 to 37f921c Compare May 20, 2022 18:57
@dariakp dariakp requested a review from durran May 20, 2022 19:18
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

Seems to be working as expected for me locally. Think this is good to go.

@dariakp dariakp merged commit c9d3816 into main May 23, 2022
@dariakp dariakp deleted the NODE-2993/implement-max-connecting branch May 23, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
4 participants