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

Add ConnectionIdlePingTime setting #461

Merged
merged 1 commit into from Mar 20, 2018
Merged

Conversation

bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 20, 2018

This allows clients to opt-out of pinging the server when a connection is retrieved from the pool, if it's been idle less than a certain amount of time. This removes a probably-unnecessary roundtrip to the server on recently-returned connections, which can improve throughput under heavy load.

Before

Job | Runtime | Toolchain | Mean | Error | StdDev | StdErr | Min | Q1 | Median | Q3 | Max |
Op/s | Gen 0 | Allocated |
---------- |-------- |-------------- |---------:|----------:|----------:|----------:|---------:|---------:|---------:|---------:|---------:|--------:|-------:|----------:|
net47 | Clr | CsProjnet47 | 184.6 us | 3.5489 us | 3.3197 us | 0.8571 us | 178.0 us | 181.9 us | 184.9 us | 186.5 us | 191.3 us | 5,416.6 | 1.7090 | 11.84 KB |
netcore20 | Core | .NET Core 2.0 | 157.1 us | 0.7065 us | 0.6263 us | 0.1674 us | 156.0 us | 156.7 us | 157.1 us | 157.5 us | 158.4 us | 6,363.6 | 1.2207 | 3.13 KB |

After

Job | Runtime | Toolchain | Mean | Error | StdDev | StdErr | Min | Q1 | Median | Q3 |
Max | Op/s | Gen 0 | Allocated |
---------- |-------- |-------------- |----------:|----------:|----------:|----------:|----------:|----------:|----------:|----------:|----------:|---------:|-------:|----------:|
net47 | Clr | CsProjnet47 | 115.20 us | 0.9376 us | 0.8771 us | 0.2265 us | 113.87 us | 114.69 us | 114.85 us | 115.97 us | 116.55 us | 8,680.5 | 1.0986 | 7.6 KB |
netcore20 | Core | .NET Core 2.0 | 91.13 us | 0.4125 us | 0.3858 us | 0.0996 us | 90.56 us | 90.66 us | 91.21 us | 91.39 us | 91.83 us | 10,973.0 | 0.9766 | 5.03 KB |

CC @sebastienros

This allows clients to opt-out of pinging the server when a connection is retrieved from the pool, if it's been idle less than a certain amount of time.
@bgrainger bgrainger merged commit 14d90bb into mysql-net:master Mar 20, 2018
2 checks passed
@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Mar 20, 2018

Added in 0.37.0.

@bgrainger bgrainger deleted the no-ping branch Mar 20, 2018
@MrSmoke
Copy link

@MrSmoke MrSmoke commented Jun 11, 2020

The docs have this setting marked as experimental. Is that still the case?

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Jun 11, 2020

Yes; it may change or be removed in the future: #596.

Changes to connection pooling (#178) may necessitate a change to this setting.

@felixpelletier
Copy link

@felixpelletier felixpelletier commented Mar 24, 2021

Our team uses the connector for many short connections to a database with a very stable connection. Having ConnectionIdlePingTime come out of the experimental phase would be great for us, as right now we have a use case where we spend about 40% of our time just pinging the server.

Should I open an issue to see if anyone else would interested in this getting out if the experimental phase? What needs to be done?

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Mar 24, 2021

Are you running >= 1.3.0 (with the new background connection reset code)?

@felixpelletier
Copy link

@felixpelletier felixpelletier commented Mar 24, 2021

Yes, 1.3.1 right now. Before the update, we spent something like 40-50% of our time resetting connections, and that seems to have helped a lot on that front. (Thank you and everyone involved for that improvement, by the way.).

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Mar 25, 2021

I think my current preference would be to make this a simple binary setting, and recommend using a retry policy (e.g., Polly) around all usages of MySqlConnector, rather than baking in just one retry policy (based on pinging the server).

That said, it does still feel quite strange that MySqlConnection.Open may or may not result in an open connection depending on the underlying connection pool:

  • no pooled connection available; has to open a new one; might be slow; >99.9% likelihood of being valid
  • pooled connection available; returns almost instantly; connection state may be invalid if the server has closed it or something else has gone wrong

OTOH, this may just be an unavoidable side-effect of how connection pooling is typically implemented for ADO.NET and not addressable until a consistent ADO.NET API for pooling is added (if ever).

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Mar 25, 2021

The available options one could choose would be:

  • reset connection upon return to pool, ping on retrieval (current default)
  • reset connection upon return to pool, no ping on retrieval
  • reset connection upon retrieval
  • no reset; ping on retrieval
  • no reset; no ping on retrieval

There are currently three connection string options to control this, and only certain combinations of them are valid; it would be nice to simplify that.

@felixpelletier
Copy link

@felixpelletier felixpelletier commented Mar 25, 2021

Yeah, an external retry policy does make way more sense. Something may break between the successful connection and the first queries, so as a library user you'd want to handle all of that yourself anyway.

As for the "Idle Time" vs binary, in our case it probably wouldn't make a difference since connections are pretty much always in use. I'm curious why you'd prefer binary, though. Haven't experimented with it yet, but having something that forgoes the ping in most cases, but still checks the connection if it's been cold for a while seems convenient, and not something you can do outside of the lib.

On the other hand, it is more complicated behavior, and always being kept on your toes about dead connections is not a bad thing.

@felixpelletier
Copy link

@felixpelletier felixpelletier commented Mar 25, 2021

Come to thing of it, wouldn't that scenario be problematic?:

  1. The connection pool gets really big with a lot of connections open. Say 50.
  2. The application does some unrelated processing for a while.
  3. The MySQL server closes all connections (timeout).
  4. The app goes back to using the database and opens a connection.
  5. The connection is bad, it tries another one, still bad.
  6. And so on until it has tried all 50 connections.

In that scenario, if the MySQL server really is dead, it would take 50 calls to Open() to make sure of it.

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Mar 25, 2021

ConnectionIdleTimeout should be cleaning up those connections in the background: a) to avoid this very problem, b) to avoid tying up resources on the server unnecessarily; a sudden spike in load shouldn't result in 50 extra connections being kept open forever.

(The default is pretty aggressive, but anything lower than MySQL's default wait_timeout value should work well in practice.)

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Mar 25, 2021

I'm curious why you'd prefer binary, though.

Mostly because it's simpler, and because I think the problem of shutting down idle connections is generally getting handled (in a gracious manner) client-side through ConnectionIdleTimeout.

@bgrainger
Copy link
Member Author

@bgrainger bgrainger commented Oct 1, 2021

I created an issue to change the default behaviour of Connection Reset=false to never ping (and just return a pooled connection): #1042.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants