Update database connections defaults #5853
Conversation
jananiarunachalam
requested review from
evan2645,
amartinezfayo,
sorindumitru,
MarcosDY and
rturner3
as code owners
jananiarunachalam
force-pushed
the
issue-5802-fix-db-conns
branch
from
b0a5681 to
9b46080
Compare
There was a problem hiding this comment.
Thank you @jananiarunachalam for working on this contribution!
| defaultConnMaxLifetime, err := time.ParseDuration("30s") | ||
| db.DB().SetConnMaxLifetime(defaultConnMaxLifetime) // default value | ||
| if err != nil { | ||
| return nil, "", false, nil, fmt.Errorf("failed to parse conn_max_lifetime %q: %w", defaultConnMaxLifetime, err) | ||
| } |
There was a problem hiding this comment.
The SetConnMaxLifetime function is already called later, based on the ConnMaxLifetime setting.
We don't need to make any change there. What we need to adjust is the ConnMaxIdleTime property calling SetConnMaxIdleTime, maybe with a value of 30s.
Also note that you will not need to make any parsing for that (i.e. time.ParseDuration("30s")), you can just call SetConnMaxIdleTime(time.Second * 30). We may define this as a constant in the function, and probably the MaxOpenConns and MaxIdleConns numbers too.
|
Also, could you update the documentation in https://github.com/spiffe/spire/blob/main/doc/plugin_server_datastore_sql.md with the new defaults? |
doc/plugin_server_datastore_sql.md
Outdated
| | max_idle_conns | The maximum number of idle connections in the pool (default: 100) | | ||
| | conn_max_lifetime | The maximum amount of time a connection may be reused (default: 30s) | |
There was a problem hiding this comment.
Actual change
jananiarunachalam
force-pushed
the
issue-5802-fix-db-conns
branch
4 times, most recently
from
7fc428b to
b9ec496
Compare
| if cfg.MaxIdleConns != nil { | ||
| db.DB().SetMaxIdleConns(*cfg.MaxIdleConns) | ||
| } | ||
| const connMaxLifetime = time.Second * 30 | ||
| db.DB().SetConnMaxLifetime(connMaxLifetime) |
There was a problem hiding this comment.
The SetConnMaxLifetime function is already called later, based on the ConnMaxLifetime setting. We shouldn't need to make any change here. What we need to adjust is the ConnMaxIdleTime property calling SetConnMaxIdleTime, maybe with a value of 30s.
doc/plugin_server_datastore_sql.md
Outdated
| | max_idle_conns | The maximum number of idle connections in the pool (default: 2) | | ||
| | conn_max_lifetime | The maximum amount of time a connection may be reused (default: unlimited) | | ||
| | max_idle_conns | The maximum number of idle connections in the pool (default: 100) | | ||
| | conn_max_lifetime | The maximum amount of time a connection may be reused (default: 30s) | |
There was a problem hiding this comment.
We don't need to make any change in the conn_max_lifetime. We need to set ConnMaxIdleTime to 30s and document it (we will not expose a setting for that).
Signed-off-by: jananiarunachalam <[email protected]>
Signed-off-by: jananiarunachalam <[email protected]>
Signed-off-by: jananiarunachalam <[email protected]>
Signed-off-by: jananiarunachalam <[email protected]>
Signed-off-by: jananiarunachalam <[email protected]>
Signed-off-by: jananiarunachalam <[email protected]>
jananiarunachalam
force-pushed
the
issue-5802-fix-db-conns
branch
from
b9ec496 to
0677af5
Compare
Signed-off-by: jananiarunachalam <[email protected]>
Pull Request check list
Affected functionality
Description of change
Which issue this PR fixes
Fixes #5802