-
Notifications
You must be signed in to change notification settings - Fork 312
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: one (*sql.DB) pool for all jobsdb #5170
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5170 +/- ##
==========================================
- Coverage 72.91% 72.90% -0.02%
==========================================
Files 439 439
Lines 51138 51194 +56
==========================================
+ Hits 37287 37321 +34
- Misses 11390 11409 +19
- Partials 2461 2464 +3 ☔ View full report in Codecov by Sentry. |
connStr := GetConnectionString(conf, componentName) | ||
db, err := sql.Open("postgres", connStr) | ||
if err != nil { | ||
return nil, fmt.Errorf("Error opening connection to database: %w", err) |
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.
return nil, fmt.Errorf("Error opening connection to database: %w", err) | |
return nil, fmt.Errorf("opening connection to database: %w", err) |
@@ -35,6 +40,74 @@ func GetConnectionString(c *config.Config, componentName string) string { | |||
) | |||
} | |||
|
|||
func GetDatabaseConnectionPool( |
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.
func GetDatabaseConnectionPool( | |
func NewDatabaseConnectionPool( |
} | ||
if err := stat.RegisterCollector( | ||
collectors.NewDatabaseSQLStats( | ||
"jobsdb", |
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.
"jobsdb", | |
componentName, |
/* | ||
TODO: find out a reasonably good value for below pool configurations | ||
*/ |
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.
/* | |
TODO: find out a reasonably good value for below pool configurations | |
*/ |
TODO: find out a reasonably good value for below pool configurations | ||
*/ | ||
|
||
maxConnsVar := conf.GetReloadableIntVar(40, 1, "JobsDB.maxOpenConnections") |
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 would avoid using JobsDB, as it is an overloaded term.
maxConnsVar := conf.GetReloadableIntVar(40, 1, "JobsDB.maxOpenConnections") | |
maxConnsVar := conf.GetReloadableIntVar(40, 1, "db.pool.maxOpenConnections") |
Or even
maxConnsVar := conf.GetReloadableIntVar(40, 1, "JobsDB.maxOpenConnections") | |
maxConnsVar := conf.GetReloadableIntVar(40, 1, "db.pool.maxOpenConnections", "db."+componentName+".pool.maxOpenConnections") |
// Not deferring close here | ||
// defer dbPool.Close() |
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.
What the reason for not closing here? Shall we inject dbPool to embeddedAppHandler. So we always use on connection pool
@@ -139,13 +139,21 @@ func (a *embeddedApp) StartRudderCore(ctx context.Context, options *app.Options) | |||
FeaturesRetryMaxAttempts: 10, | |||
}) | |||
|
|||
dbPool, err := misc.GetDatabaseConnectionPool(ctx, config, statsFactory, "jobsdb") |
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.
dbPool, err := misc.GetDatabaseConnectionPool(ctx, config, statsFactory, "jobsdb") | |
dbPool, err := misc.GetDatabaseConnectionPool(ctx, config, statsFactory, "embedded-app") |
I would avoid using jobsdb
for anything else other than jobsdb component
Description
One connection pool for all jobsdb.
In future, use the same pool across other areas(reporting, backend-config cache, drain-config etc. with appropriate increase in pool configurations)
Using given config(all of these are hot-reloadable):
maxOpenConnections
: 40maxIdleConnections
: 5maxIdleConnIdleTime
: 15 MinutesmaxConnLifetime
: 0Linear Ticket
Resolves PIPE-1602
Security