-
Notifications
You must be signed in to change notification settings - Fork 246
DRIVERS-3218 Avoid clearing the connection pool when the server connection rate limiter triggers #1852
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
base: master
Are you sure you want to change the base?
DRIVERS-3218 Avoid clearing the connection pool when the server connection rate limiter triggers #1852
Changes from 14 commits
4e6bfeb
d830b12
c6c8626
70de630
51196d8
3d27dd4
b72e044
20cc961
dc6797b
e72968a
91cd80f
b0561bd
5b37566
a56e7ec
4c0df36
39acfd2
fdb46c6
8b0d35d
a8ffc2a
db4b1a3
c7608cc
ab2eb87
7309f40
b349f0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| version: 1 | ||
| style: integration | ||
| description: pool enters backoff on connection close | ||
| runOn: | ||
| - minServerVersion: 4.9.0 | ||
| failPoint: | ||
| configureFailPoint: failCommand | ||
| mode: | ||
| times: 1 | ||
| data: | ||
| failCommands: | ||
| - isMaster | ||
| - hello | ||
| closeConnection: true | ||
| poolOptions: | ||
| minPoolSize: 0 | ||
| operations: | ||
| - name: ready | ||
| - name: start | ||
| target: thread1 | ||
| - name: checkOut | ||
| thread: thread1 | ||
| - name: waitForEvent | ||
| event: ConnectionCreated | ||
| count: 1 | ||
| - name: waitForEvent | ||
| event: ConnectionCheckOutFailed | ||
| count: 1 | ||
| events: | ||
| - type: ConnectionCheckOutStarted | ||
| - type: ConnectionCreated | ||
| - type: ConnectionClosed | ||
| - type: ConnectionPoolBackoff | ||
| - type: ConnectionCheckOutFailed | ||
| ignore: | ||
| - ConnectionCheckedIn | ||
| - ConnectionCheckedOut | ||
| - ConnectionPoolCreated | ||
| - ConnectionPoolReady |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,39 +2,37 @@ version: 1 | |
| style: integration | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't modify spec tests like this anymore, because that will break for drivers using submodules to track spec tests who haven't implemented backoff yet, and if those drivers then skip these tests they lose coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm this is a tricky one, because we're changing the behavior of the driver. So I guess we need a new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, something like that would work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh. And we should also do the inverse for the tests we're leaving alone: runOnRequirement of !supportsPoolBackoff. (I just spent some time debugging a failing SDAM test on my branch only to realize it was supposed to fail with my changes). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added |
||
| description: error during minPoolSize population clears pool | ||
| runOn: | ||
| - | ||
| # required for appName in fail point | ||
| minServerVersion: "4.9.0" | ||
| - minServerVersion: 4.9.0 | ||
| failPoint: | ||
| configureFailPoint: failCommand | ||
| # high amount to ensure not interfered with by monitor checks. | ||
| mode: { times: 50 } | ||
| mode: alwaysOn | ||
| data: | ||
| failCommands: ["isMaster","hello"] | ||
| closeConnection: true | ||
| appName: "poolCreateMinSizeErrorTest" | ||
| failCommands: | ||
| - isMaster | ||
| - hello | ||
| errorCode: 18 | ||
| appName: poolCreateMinSizeErrorTest | ||
| poolOptions: | ||
| minPoolSize: 1 | ||
| backgroundThreadIntervalMS: 50 | ||
| appName: "poolCreateMinSizeErrorTest" | ||
| appName: poolCreateMinSizeErrorTest | ||
| operations: | ||
| - name: ready | ||
| - name: waitForEvent | ||
| event: ConnectionPoolCleared | ||
| count: 1 | ||
| # ensure pool doesn't start making new connections | ||
| - name: wait | ||
| ms: 200 | ||
| events: | ||
| - type: ConnectionPoolReady | ||
| address: 42 | ||
| - type: ConnectionCreated | ||
| address: 42 | ||
| - type: ConnectionPoolCleared | ||
| address: 42 | ||
| - type: ConnectionClosed | ||
| address: 42 | ||
| connectionId: 42 | ||
| reason: error | ||
| - type: ConnectionPoolCleared | ||
| address: 42 | ||
| ignore: | ||
| - ConnectionPoolCreated | ||
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 was talking with @ShaneHarvey about this (related comments on drivers ticket). Shane's understanding is that we decided to include all timeout errors, regardless of where it originated, during connection establishment. Does that match your understanding, Steve?
And related; the design says:
We should update the design with whatever the outcome of this thread is.
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.
Yeah that's a good callout, but it can't be from auth, since the auth spec explicitly calls out the timeout behavior. I'm assuming all drivers can distinguish between hello and auth since the are separate commands. I'll update to say if the driver can distinguish between TCP connect/DNS and the TLS handshake then it MUST do so.
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.
Which timeout behavior in the auth spec are you referring to? I searched for
timeoutand only saw stuff about what timeout values to use, but not how to handle network timeouts. Maybe I'm looking in the wrong place though.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.
Ah, the specs must have gotten out of sync. I'm referring to:
https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring.md#why-mark-a-server-unknown-after-an-auth-error
"The Authentication spec requires that when authentication fails on a server, the driver MUST clear the server's connection pool"