Conversation
There was a problem hiding this comment.
looplj has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Code Review
This pull request introduces configurable database connection pool settings, including maximum open and idle connections, as well as connection lifetime and idle time limits. It also updates the pgx dependency and disables window focus refetching for the useRequests hook in the frontend. Feedback includes a suggestion to remove the > 0 checks when applying database configurations to allow for zero-value settings (e.g., disabling pooling) and a recommendation to apply the refetchOnWindowFocus optimization consistently across other hooks in the frontend.
| if cfg.MaxOpenConns > 0 { | ||
| sqlDB.SetMaxOpenConns(cfg.MaxOpenConns) | ||
| } | ||
|
|
||
| if cfg.MaxIdleConns > 0 { | ||
| sqlDB.SetMaxIdleConns(cfg.MaxIdleConns) | ||
| } | ||
|
|
||
| if cfg.ConnMaxLifetime > 0 { | ||
| sqlDB.SetConnMaxLifetime(cfg.ConnMaxLifetime) | ||
| } | ||
|
|
||
| if cfg.ConnMaxIdleTime > 0 { | ||
| sqlDB.SetConnMaxIdleTime(cfg.ConnMaxIdleTime) | ||
| } |
There was a problem hiding this comment.
The checks for > 0 prevent setting these parameters to their 'zero' values (e.g., unlimited or disabled). Specifically for MaxIdleConns, the default in Go's sql.DB is 2. If a user explicitly wants to disable connection pooling by setting max_idle_conns: 0 in the configuration, this check will skip the call, leaving the pool with the default 2 idle connections. Since default values are provided in the configuration loader, these fields will always be populated. It is cleaner and more correct to apply the values directly.
| if cfg.MaxOpenConns > 0 { | |
| sqlDB.SetMaxOpenConns(cfg.MaxOpenConns) | |
| } | |
| if cfg.MaxIdleConns > 0 { | |
| sqlDB.SetMaxIdleConns(cfg.MaxIdleConns) | |
| } | |
| if cfg.ConnMaxLifetime > 0 { | |
| sqlDB.SetConnMaxLifetime(cfg.ConnMaxLifetime) | |
| } | |
| if cfg.ConnMaxIdleTime > 0 { | |
| sqlDB.SetConnMaxIdleTime(cfg.ConnMaxIdleTime) | |
| } | |
| sqlDB.SetMaxOpenConns(cfg.MaxOpenConns) | |
| sqlDB.SetMaxIdleConns(cfg.MaxIdleConns) | |
| sqlDB.SetConnMaxLifetime(cfg.ConnMaxLifetime) | |
| sqlDB.SetConnMaxIdleTime(cfg.ConnMaxIdleTime) |
| } | ||
| }, | ||
| enabled, | ||
| refetchOnWindowFocus: false, |
There was a problem hiding this comment.
The refetchOnWindowFocus: false optimization is applied to the useRequests hook but not to other similar hooks in this file, such as useRequestExecutions. Consider applying this setting consistently across the feature to ensure uniform behavior and optimize performance.
Uh oh!
There was an error while loading. Please reload this page.