Skip to content
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

feat: add task to release unused db conns #1640

Merged
merged 9 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions syncserver/src/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,13 @@ impl Server {
&Metrics::from(&metrics),
blocking_threadpool.clone(),
)?;
// Spawns sweeper that calls Deadpool `retain` method, clearing unused connections.
db_pool.spawn_sweeper(Duration::from_secs(
settings
.syncstorage
.database_pool_sweeper_task_interval
.into(),
));
let glean_logger = Arc::new(GleanEventsLogger {
// app_id corresponds to probe-scraper entry.
// https://github.com/mozilla/probe-scraper/blob/main/repositories.yaml
Expand Down
14 changes: 14 additions & 0 deletions syncstorage-mysql/src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ impl MysqlDbPool {
})
}

/// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sweeper()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can drop this and make spawn_sweeper(..) the noop in MysqlDb.

}

pub fn get_sync(&self) -> DbResult<MysqlDb> {
Ok(MysqlDb::new(
self.pool.get()?,
Expand All @@ -110,6 +117,13 @@ impl MysqlDbPool {
}
}

/// Sweeper to retain only the objects specified within the closure.
/// In this context, if a Spanner connection is unutilized, we want it
/// to release the given connections.
/// See: https://docs.rs/deadpool/latest/deadpool/managed/struct.Pool.html#method.retain
/// Noop for mysql impl
fn sweeper() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's drop this. Less functions, less problems.


#[async_trait]
impl DbPool for MysqlDbPool {
type Error = DbError;
Expand Down
3 changes: 3 additions & 0 deletions syncstorage-settings/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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: u32,
#[cfg(debug_assertions)]
pub database_use_test_transactions: bool,
#[cfg(debug_assertions)]
Expand Down Expand Up @@ -115,6 +117,7 @@ impl Default for Settings {
database_pool_min_idle: None,
database_pool_connection_lifespan: None,
database_pool_connection_max_idle: None,
database_pool_sweeper_task_interval: 30,
database_pool_connection_timeout: Some(30),
#[cfg(debug_assertions)]
database_use_test_transactions: false,
Expand Down
1 change: 1 addition & 0 deletions syncstorage-spanner/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ authors.workspace = true
edition.workspace = true

[dependencies]
actix-web.workspace = true
backtrace.workspace = true
cadence.workspace = true
deadpool.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion syncstorage-spanner/src/manager/deadpool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::error::DbError;
pub(crate) type Conn = deadpool::managed::Object<SpannerSessionManager>;

pub(crate) struct SpannerSessionManager {
settings: SpannerSessionSettings,
pub settings: SpannerSessionSettings,
/// The gRPC environment
env: Arc<Environment>,
metrics: Metrics,
Expand Down
29 changes: 28 additions & 1 deletion syncstorage-spanner/src/pool.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{collections::HashMap, fmt, sync::Arc, time::Duration};

use actix_web::rt;
use async_trait::async_trait;
use syncserver_common::{BlockingThreadpool, Metrics};
use syncserver_db_common::{GetPoolState, PoolState};
Expand Down Expand Up @@ -49,7 +50,9 @@ impl SpannerDbPool {
let config = deadpool::managed::PoolConfig {
max_size,
timeouts,
..Default::default()
// Prefer LIFO to allow the sweeper task to evict least frequently
// used connections.
queue_mode: deadpool::managed::QueueMode::Lifo,
};
let pool = deadpool::managed::Pool::builder(manager)
.config(config)
Expand Down Expand Up @@ -84,6 +87,30 @@ impl SpannerDbPool {
self.quota,
))
}

/// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do move this to the impl DbPool trait, you can keep the fn sweeper() as part of the general impl block.

let Some(max_idle) = self.pool.manager().settings.max_idle else {
return;
};
let pool = self.pool.clone();
rt::spawn(async move {
loop {
sweeper(&pool, Duration::from_secs(max_idle.into()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rt::time::sleep(interval).await;
}
});
}
}

/// Sweeper to retain only the objects specified within the closure.
/// In this context, if a Spanner connection is unutilized, we want it
/// to release the given connection.
/// See: https://docs.rs/deadpool/latest/deadpool/managed/struct.Pool.html#method.retain
fn sweeper(pool: &deadpool::managed::Pool<SpannerSessionManager>, max_idle: Duration) {
pool.retain(|_, metrics| metrics.last_used() < max_idle);
}

#[async_trait]
Expand Down