feat(NODE-3011): Load Balancer Support #2909

durran · 2021-07-15T17:28:06Z

No description provided.

dariakp

I got through almost half of the PR, going to take a fresh look at the rest of it Monday; meanwhile, some thoughts/questions

package.json

src/cmap/connect.ts

src/cmap/connection.ts

src/cmap/errors.ts

src/cmap/metrics.ts

test/tools/runner/index.js

dariakp

Some more questions/comments, still not quite finished with the PR because I want to take a very close look at connection pool and sessions changes

src/error.ts

src/cursor/abstract_cursor.ts

src/operations/command.ts

test/unit/cmap/metrics.test.js

test/unit/mongo_client_options.test.js

test/unit/cmap/stream_description.test.js

emadum

Did a full read through and left a few nit comments - I'll follow up with a final pass once some of the active comments on the PR have been resolved. Very nice work here! 👍

src/transactions.ts

test/functional/spec-runner/index.js

dariakp

I went through and resolved the comments that were fully addressed; there are still a few that don't have replies (they might have gotten hidden by github on the main page) and some others that are open for discussion

src/cmap/connection_pool.ts

test/unit/cmap/stream_description.test.js

src/cmap/connection.ts

test/unit/mongo_client_options.test.js

src/utils.ts

src/sessions.ts

nbbeeken

I just merged in the changes stream test changes, so you can sync those in making the PR a bit smaller.
Got some nits and some more concept questions here:

src/cmap/connect.ts

src/cmap/connection.ts

src/cmap/connection_pool.ts

test/functional/unified-spec-runner/runner.ts

test/functional/change_stream_spec.test.js

test/manual/load-balancer.test.js

test/tools/mock.js

nbbeeken

My review request got cleared early I think, there are still comments outstanding (maybe collapsed by GH) I resolved what's been addressed hopefully that makes things more visible

src/utils.ts

src/sdam/server.ts

dariakp

I went through and resolved all of my comments that didn't require follow up. There's a few things left on outstanding TODOs, but I think we're pretty close

dariakp

Accidentally cleared my review, resetting to "request changes" for the unresolved comments

dariakp

Just following up on a couple of questions, resolved most outstanding comments

src/sessions.ts

src/cursor/abstract_cursor.ts

test/functional/unified-spec-runner/runner.ts

nbbeeken

Just looking for some discussion on these:

kProcessingWaitQueue question
Throw to prevent double unshift?
And I replied to the throwing when maybeClearingPinned

dariakp · 2021-07-29T20:18:52Z

LGTM pending merge conflict resolution and green tests

src/cursor/abstract_cursor.ts

nbbeeken · 2021-08-02T17:43:55Z

src/operations/aggregate.ts

@@ -79,6 +79,10 @@ export class AggregateOperation<T = Document> extends CommandOperation<T> {
    }
  }

+  get isCursorCreating(): boolean {


This seems it would be better suited to be an aspect. If we could refactor to that it would be great but also something we can defer as well.

Do we actually need this on operations classes? The cursor design is such that subclasses of AbstractCursor implement their own initialization which execute an operation, so we should have all the context we need about which operations create cursors without explicitly adding that context to operations classes. Is there a reason you can't move the check in executeOperation into the AbstractCursor call to _initialize ?

Well the addition of this was specifically for retry behaviour, and I wanted to keep all the retry related code in the same place and not have it scattered in multiple places.

Also because of the findOne case - it doesn't directly instantiate a cursor, it calls collection.find in its operation which means that the session in that case it not owned by the cursor as executeOperation creates the implicit session, not cursor._initialize.

Refactored in a026ce4 to use aspects.

What tests were failing without this condition? I tried running the tests locally without the creates_cursor aspect and all passed. There are some skips so maybe my environment is incomplete. (I have both multi and single env variables)

No tests fail without this condition, but it leaks the connection each time findOne retries. This is because findOne calls executeOperation, which itself creates an implicit session with the owner being a symbol. Then inside its execute method it calls collection.find, which creates a cursor with the already created implicit session. The connection is pinned at the connection pool level because find is a pinnable operation, and then when the cursor exhausts it does not unpin because the cursor is not the owner of the session. So the connection remains pinned to the session.

Here are the extra 2 checked out connections from the findOne retries at the end of the test run.

I do agree in this case that the pin should never have occurred, but until we refactor the operation layer it's always going to be pinned here because we pin on connection checkout from the pool and the find command is pinnable.

Also we have already set a precedent for doing logic like this in executeOperation: https://github.com/mongodb/node-mongodb-native/pull/2909/files#diff-f879df64594f4d352859a191839a674ff7a497d6a41f8ccfc71d965b48c6d3d6R144

emadum

Couple of small suggestions but this is looking good to me. 🥇

test/functional/unified-spec-runner/runner.ts

src/sdam/server.ts

Co-authored-by: Eric Adum <[email protected]>

durran force-pushed the NODE-3011/load-balancer-support branch 3 times, most recently from b8c5ac1 to c536a22 Compare July 16, 2021 14:13

nbbeeken requested review from nbbeeken, dariakp and emadum July 16, 2021 17:15

dariakp reviewed Jul 16, 2021

View reviewed changes

dariakp added the Team Review Needs review from team label Jul 19, 2021

dariakp reviewed Jul 19, 2021

View reviewed changes

emadum suggested changes Jul 20, 2021

View reviewed changes

src/transactions.ts Outdated Show resolved Hide resolved

test/functional/spec-runner/index.js Outdated Show resolved Hide resolved

durran requested review from emadum and dariakp July 20, 2021 18:19

dariakp requested changes Jul 20, 2021

View reviewed changes

nbbeeken requested changes Jul 21, 2021

View reviewed changes

durran requested a review from nbbeeken July 22, 2021 14:13

durran force-pushed the NODE-3011/load-balancer-support branch from b996d70 to b8687dd Compare July 23, 2021 13:53

nbbeeken requested changes Jul 23, 2021

View reviewed changes

dariakp reviewed Jul 23, 2021

View reviewed changes

src/utils.ts Outdated Show resolved Hide resolved

src/sdam/server.ts Outdated Show resolved Hide resolved

dariakp approved these changes Jul 23, 2021

View reviewed changes

dariakp self-requested a review July 23, 2021 18:29

dariakp requested changes Jul 23, 2021

View reviewed changes

durran requested review from nbbeeken and dariakp July 26, 2021 12:14

dariakp reviewed Jul 26, 2021

View reviewed changes

src/sessions.ts Show resolved Hide resolved

src/cursor/abstract_cursor.ts Show resolved Hide resolved

test/functional/unified-spec-runner/runner.ts Outdated Show resolved Hide resolved

nbbeeken requested changes Jul 26, 2021

View reviewed changes

durran changed the title ~~NODE-3011: Load Balancer Support~~ feat(NODE-3011): Load Balancer Support Jul 28, 2021

durran requested review from nbbeeken and dariakp July 29, 2021 14:09

durran force-pushed the NODE-3011/load-balancer-support branch from 453fc4c to a1f4489 Compare July 30, 2021 14:28

durran force-pushed the NODE-3011/load-balancer-support branch 6 times, most recently from 212cc48 to 78a8b9b Compare July 30, 2021 22:30

durran marked this pull request as ready for review July 30, 2021 22:30

durran force-pushed the NODE-3011/load-balancer-support branch 7 times, most recently from 302eef1 to a7c188a Compare August 2, 2021 15:29

nbbeeken reviewed Aug 2, 2021

View reviewed changes

durran requested a review from nbbeeken August 2, 2021 18:35

durran force-pushed the NODE-3011/load-balancer-support branch from 363d41b to 9168bbc Compare August 2, 2021 20:30

durran added 4 commits August 3, 2021 16:16

feat(loadBalancer)!: add support for load balancers

5dec243

fix: unpin on first execution of cursor creating command

bed9fae

refactor: use aspects instead of isCursorCreating

794fe7a

doc: update comment on interrupted error check

8ab4cfa

durran force-pushed the NODE-3011/load-balancer-support branch from 9168bbc to 8ab4cfa Compare August 3, 2021 14:17

nbbeeken approved these changes Aug 3, 2021

View reviewed changes

emadum approved these changes Aug 3, 2021

View reviewed changes

test/functional/unified-spec-runner/runner.ts Outdated Show resolved Hide resolved

src/sdam/server.ts Show resolved Hide resolved

dariakp approved these changes Aug 3, 2021

View reviewed changes

nbbeeken and others added 2 commits August 3, 2021 15:52

add ticket number to TODO

179f205

Co-authored-by: Eric Adum <[email protected]>

Callout about monitoring

6ff2c86

Co-authored-by: Eric Adum <[email protected]>

nbbeeken merged commit c554a7a into 4.0 Aug 3, 2021

nbbeeken deleted the NODE-3011/load-balancer-support branch August 3, 2021 20:12

ljhaywar pushed a commit that referenced this pull request Nov 9, 2021

feat(NODE-3011): Load Balancer Support (#2909)

4f32db0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-3011): Load Balancer Support #2909

feat(NODE-3011): Load Balancer Support #2909

durran commented Jul 15, 2021

dariakp left a comment

dariakp left a comment

emadum left a comment

dariakp left a comment

nbbeeken left a comment

nbbeeken left a comment •

edited

Loading

dariakp left a comment

dariakp left a comment

dariakp left a comment

nbbeeken left a comment •

edited

Loading

dariakp commented Jul 29, 2021

nbbeeken Aug 2, 2021

mbroadst Aug 2, 2021

durran Aug 2, 2021

durran Aug 2, 2021 •

edited

Loading

durran Aug 2, 2021

nbbeeken Aug 2, 2021

durran Aug 3, 2021 •

edited

Loading

durran Aug 3, 2021

durran Aug 3, 2021

durran Aug 3, 2021

emadum left a comment

feat(NODE-3011): Load Balancer Support #2909

feat(NODE-3011): Load Balancer Support #2909

Conversation

durran commented Jul 15, 2021

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

emadum left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

nbbeeken left a comment • edited Loading

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

nbbeeken left a comment • edited Loading

Choose a reason for hiding this comment

dariakp commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durran Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durran Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emadum left a comment

Choose a reason for hiding this comment

nbbeeken left a comment •

edited

Loading

nbbeeken left a comment •

edited

Loading

durran Aug 2, 2021 •

edited

Loading

durran Aug 3, 2021 •

edited

Loading