feat: add task to release unused db conns #1640
Conversation
taddes
force-pushed
the
feat/add-task-to-release-unused-db-conns-SYNC-4542
branch
from
cb9ceb5 to
8f4a11b
Compare
syncstorage-settings/src/lib.rs
Outdated
| @@ -78,6 +78,8 @@ pub struct Settings { | |||
| pub database_pool_connection_lifespan: Option<u32>, | |||
| /// Max time a connection should sit idle before being dropped. | |||
| pub database_pool_connection_max_idle: Option<u32>, | |||
| /// Interval for sweeper task releasing unused connections. | |||
| pub database_pool_sweeper_task_interval: Option<u32>, | |||
There was a problem hiding this comment.
Let's kill the Option, it's a bit of an anti pattern with a default of Some -- because there's no way to force it to a None (at least not from an environment variable) that I know of
| pub database_pool_sweeper_task_interval: Option<u32>, | |
| pub database_pool_sweeper_task_interval: u32, |
There was a problem hiding this comment.
@pjenvey you ok with 30 s as a sweeper interval?
There was a problem hiding this comment.
Yea, whatever works for autopush should be fine for syncstorage
| /// Interval for sweeper task releasing unused connections. | ||
| pub pool_max_idle: Option<u32>, |
There was a problem hiding this comment.
We already have a database_pool_connection_max_idle setting, let's use that instead -- the pool implementation uses it to timeout connections as it checks them out, but I believe the sweeper's going to do a better job of reducing the number than that code
There was a problem hiding this comment.
You bet @pjenvey , I wasn't 100% sure about reusing some of those settings so opted to just make a new one.
There was a problem hiding this comment.
I think we can kill it entirely after using the other setting?
| let pool = self.pool.clone(); | ||
| rt::spawn(async move { | ||
| loop { | ||
| sweeper(&pool, Duration::from_secs(max_idle.into())); |
There was a problem hiding this comment.
in general, prefer constructing something like this Duration outside of the loop -- but it's a small, Copy type anyway so it doesn't really matter at all here
| /// Spawn a task to periodically evict idle connections. Calls wrapper sweeper fn | ||
| /// to use pool.retain, retaining objects only if they are shorter in duration than | ||
| /// defined max_idle. Noop for mysql impl. | ||
| pub fn spawn_sweeper(&self, _interval: Duration) { |
There was a problem hiding this comment.
If this is a function that all implementations of DbPool should perform, we should add it to the DbPool trait (and move this to the impl DbPool block.
I wonder if we should rename this to spawn_maintenance (spawn_janitor?)
While we don't need to sweep for pool connections for SQL libs, folk may want to do other maintenance things like run vacuum or the like.
| /// to use pool.retain, retaining objects only if they are shorter in duration than | ||
| /// defined max_idle. Noop for mysql impl. | ||
| pub fn spawn_sweeper(&self, _interval: Duration) { | ||
| sweeper() |
There was a problem hiding this comment.
I think we can drop this and make spawn_sweeper(..) the noop in MysqlDb.
| /// to release the given connections. | ||
| /// See: https://docs.rs/deadpool/latest/deadpool/managed/struct.Pool.html#method.retain | ||
| /// Noop for mysql impl | ||
| fn sweeper() {} |
There was a problem hiding this comment.
Yeah, let's drop this. Less functions, less problems.
| /// Spawn a task to periodically evict idle connections. Calls wrapper sweeper fn | ||
| /// to use pool.retain, retaining objects only if they are shorter in duration than | ||
| /// defined max_idle. | ||
| pub fn spawn_sweeper(&self, interval: Duration) { |
There was a problem hiding this comment.
If you do move this to the impl DbPool trait, you can keep the fn sweeper() as part of the general impl block.
|
Let's add a new trait method for |
taddes
changed the title
Description
By default, Deadpool releases unused connections pretty lazily, causing connections that may otherwise be available to be tied up unnecessarily. This could be part of what’s causing pods to run out of available connections to Spanner in certain cases. To resolve this, we should pursue a solution like the one described here, where we spawn a background task that periodically releases connections that haven’t been used within a given amount of time (this amount of time should be configurable in a setting).
To do this, we’ll need to make use of the Pool::retain method, which was introduced in deadpool 0.9.3 (we’re currently on version 0.5, so we’ll need to upgrade).
Testing
How should reviewers test?
Issue(s)
Closes SYNC-4542.