-
Notifications
You must be signed in to change notification settings - Fork 301
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
chore: tune shared db connection pooling #4213
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the TipsChat with CodeRabbit Bot (
|
5c08fac
to
2745564
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v1.18.x #4213 +/- ##
===================================================
- Coverage 74.40% 72.66% -1.74%
===================================================
Files 397 387 -10
Lines 58330 56488 -1842
===================================================
- Hits 43399 41049 -2350
- Misses 12623 13068 +445
- Partials 2308 2371 +63 ☔ View full report in Codecov by Sentry. |
@@ -192,5 +192,7 @@ func setupDBConn(conf *config.Config) (*sql.DB, error) { | |||
if err := db.Ping(); err != nil { | |||
return nil, fmt.Errorf("db ping: %v", err) | |||
} | |||
db.SetMaxIdleConns(conf.GetInt("drainConfig.maxIdleConns", 1)) | |||
db.SetMaxOpenConns(conf.GetInt("drainConfig.maxOpenConns", 2)) |
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.
We could get away with db.SetMaxOpenConns(1)
if we remove the two drainConfig routines from gatewayAppHandler.
As for processor, both routines can carry on with one connection imo.
@@ -202,6 +203,9 @@ func NewJobService(config JobServiceConfig) (JobService, error) { | |||
if config.MaxPoolSize <= 2 { | |||
config.MaxPoolSize = 2 // minimum 2 connections in the pool for proper startup | |||
} | |||
if config.MinPoolSize <= 0 { |
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.
why not an upper limit for both of them too..?
maxConns := lo.Clamp(config.MaxPoolSize, 2, 3)
minConss := lo.Clamp(config.MinPoolSize, 1, 2)
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 don't think we need to limit a configuration option that is there just for overriding it in case we face some issue in production
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.
true
Description
Trying to minimise each rudder-server's footprint on the overall number of shared postgres's connections
Security