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

database/sql: make maxBadConnRetries configurable #48309

Open
thameezb opened this issue Sep 10, 2021 · 3 comments
Open

database/sql: make maxBadConnRetries configurable #48309

thameezb opened this issue Sep 10, 2021 · 3 comments
Labels
NeedsInvestigation

Comments

@thameezb
Copy link

@thameezb thameezb commented Sep 10, 2021

What version of Go are you using (go version)?

v1.16.2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

n/a

What did you do?

The value of maxBadConnRetries is hardcoded to 2 -> https://github.com/golang/go/blob/master/src/database/sql/sql.go#L1511.
I was wondering if there is a specific reason for this, as there seems to be attempts to make this variable in the past which were abandoned -> #15608

Making it variable would be very much appreciated in our particular use-case.

Scenario

  • Running a k8s cluster with various services running as pods in the cluster. Each of these pods has a sidecar Health-Monitor container which simply pings certain dependencies on a regular interval (10s). Should the health-check fail, the dependency is marked as down and potentially the pod can be marked as not-ready.
  • One of our dependency types is SQL, and the go-sql lib is used.
  • There are times where we need to bring down our DB (during patching etc)
    • and during these times we see connections to the DB spike to triple in traffic (makes sense now that we have looked at the code)
    • this causes the DB instance to be hit hard and load to increase, during a period where load is already high due to an external process
    • this also means that the HM acts differently according to different scenarios (happy/sad)

Note that we cannot simply block connections to the entire DB instance as it needs to be contactable during this period.

@seankhliao seankhliao changed the title Make maxBadConnRetries configurable database/sql: make maxBadConnRetries configurable Sep 10, 2021
@seankhliao seankhliao added the NeedsInvestigation label Sep 10, 2021
@seankhliao
Copy link
Member

@seankhliao seankhliao commented Sep 10, 2021

cc @bradfitz @kardianos

@thameezb
Copy link
Author

@thameezb thameezb commented Sep 25, 2021

any update?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 25, 2021

There are two ways to approach this, especially that we do a better job of marking connections as bad before they go into the pool to begin with.

  1. Add a configuration with the driver to not return driver.ErrBadConn
  2. Add a configuration with database/sql to not retry.

I'm open to (2), but I would prefer it not to introduce more locks and shared state. Want to suggest a change?

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

No branches or pull requests

3 participants