feat: adapt PoolState to usage of deadpool everywhere (#2128)

just killing it for deadpool::Status since it provides a public constructor

and kill the long unused syncstorage-spanner bb8 based manager

Closes STOR-368

Author: pjenvey

Cargo.lock

@@ -1,7 +1,7 @@
 pub mod error;
 pub mod test;
 
-use std::{error::Error, fmt::Debug};
+use std::error::Error;
 
 #[cfg(debug_assertions)]
 use diesel::connection::InstrumentationEvent;
@@ -14,25 +14,9 @@ use diesel_migrations::{EmbeddedMigrations, MigrationHarness};
 use tokio::task::spawn_blocking;
 
 /// A trait to be implemented by database pool data structures. It provides an interface to
-/// derive the current state of the pool, as represented by the `PoolState` struct.
-pub trait GetPoolState {
-    fn state(&self) -> PoolState;
-}
-
-#[derive(Debug, Default)]
-/// A mockable r2d2::State
-pub struct PoolState {
-    pub connections: u32,
-    pub idle_connections: u32,
-}
-
-impl From<deadpool::Status> for PoolState {
-    fn from(status: deadpool::Status) -> PoolState {
-        PoolState {
-            connections: status.size as u32,
-            idle_connections: status.available.max(0) as u32,
-        }
-    }
+/// derive the current status of the pool, as represented by [deadpool::Status]
+pub trait GetPoolStatus {
+    fn status(&self) -> deadpool::Status;
 }
 
 /// Establish an [AsyncConnection] logging diesel queries to the `debug` log

syncserver-db-common/src/lib.rs

@@ -12,6 +12,7 @@ backtrace.workspace = true
 base64.workspace = true
 cadence.workspace = true
 chrono.workspace = true
+deadpool.workspace = true
 docopt.workspace = true
 futures.workspace = true
 hex.workspace = true

syncserver/Cargo.toml

@@ -19,7 +19,7 @@ use syncserver_common::{
     BlockingThreadpool, BlockingThreadpoolMetrics, Metrics, Taggable,
     middleware::sentry::SentryWrapper,
 };
-use syncserver_db_common::{GetPoolState, PoolState};
+use syncserver_db_common::GetPoolStatus;
 use syncserver_settings::Settings;
 use syncstorage_db::{DbError, DbPool, DbPoolImpl};
 use syncstorage_settings::{Deadman, ServerLimits};
@@ -575,7 +575,7 @@ impl FromRequest for MetricsWrapper {
 }
 
 /// Emit database pool and threadpool metrics periodically
-fn spawn_metric_periodic_reporter<T: GetPoolState + Send + 'static>(
+fn spawn_metric_periodic_reporter<T: GetPoolStatus + Send + 'static>(
     interval: Duration,
     metrics: Arc<StatsdClient>,
     pool: T,
@@ -603,22 +603,14 @@ fn spawn_metric_periodic_reporter<T: GetPoolState + Send + 'static>(
         };
 
         loop {
-            let PoolState {
-                connections,
-                idle_connections,
-            } = pool.state();
+            let deadpool::Status {
+                size, available, ..
+            } = pool.status();
             send_gauge_with_maybe_hostname(
                 "storage.pool.connections.active",
-                (connections - idle_connections) as u64,
-            );
-            send_gauge_with_maybe_hostname(
-                "storage.pool.connections.active",
-                (connections - idle_connections) as u64,
-            );
-            send_gauge_with_maybe_hostname(
-                "storage.pool.connections.idle",
-                idle_connections as u64,
+                (size - available) as u64,
             );
+            send_gauge_with_maybe_hostname("storage.pool.connections.idle", available as u64);
 
             let BlockingThreadpoolMetrics {
                 queued_tasks,

syncserver/src/server/mod.rs

@@ -846,49 +846,52 @@ pub async fn lbheartbeat(req: HttpRequest) -> Result<HttpResponse, ApiError> {
     let db_state = if cfg!(test) {
         use actix_web::http::header::HeaderValue;
         use std::str::FromStr;
-        use syncstorage_db::PoolState;
-
-        PoolState {
-            connections: u32::from_str(
-                req.headers()
-                    .get("TEST_CONNECTIONS")
-                    .unwrap_or(&HeaderValue::from_static("0"))
-                    .to_str()
-                    .unwrap_or("0"),
-            )
-            .unwrap_or_default(),
-            idle_connections: u32::from_str(
-                req.headers()
-                    .get("TEST_IDLES")
-                    .unwrap_or(&HeaderValue::from_static("0"))
-                    .to_str()
-                    .unwrap_or("0"),
-            )
-            .unwrap_or_default(),
+
+        let size = usize::from_str(
+            req.headers()
+                .get("TEST_CONNECTIONS")
+                .unwrap_or(&HeaderValue::from_static("0"))
+                .to_str()
+                .unwrap_or("0"),
+        )
+        .unwrap_or_default();
+        let available = usize::from_str(
+            req.headers()
+                .get("TEST_IDLES")
+                .unwrap_or(&HeaderValue::from_static("0"))
+                .to_str()
+                .unwrap_or("0"),
+        )
+        .unwrap_or_default();
+        deadpool::Status {
+            max_size: size,
+            size,
+            available,
+            waiting: 0,
         }
     } else {
-        state.db_pool.clone().state()
+        state.db_pool.status()
     };
 
-    let active = db_state.connections - db_state.idle_connections;
+    let active = db_state.size - db_state.available;
     let mut status_code = StatusCode::OK;
 
-    if active >= deadman.max_size && db_state.idle_connections == 0 {
+    if active >= deadman.max_size as usize && db_state.available == 0 {
         if deadman.clock_start.is_none() {
             deadman.clock_start = Some(Instant::now());
         }
         status_code = StatusCode::INTERNAL_SERVER_ERROR;
     } else if deadman.clock_start.is_some() {
         deadman.clock_start = None
     }
-    deadman.previous_count = db_state.idle_connections as usize;
+    deadman.previous_count = db_state.available;
     {
         *deadarc.write().await = deadman;
     }
     resp.insert("active_connections".to_string(), Value::from(active));
     resp.insert(
         "idle_connections".to_string(),
-        Value::from(db_state.idle_connections),
+        Value::from(db_state.available),
     );
     if let Some(clock) = deadman.clock_start {
         let duration: Duration = Instant::now() - clock;

syncserver/src/web/handlers.rs

@@ -10,7 +10,7 @@ use std::fmt::Debug;
 use async_trait::async_trait;
 use lazy_static::lazy_static;
 use serde::Deserialize;
-use syncserver_db_common::GetPoolState;
+use syncserver_db_common::GetPoolStatus;
 
 use error::DbErrorIntrospect;
 use util::SyncTimestamp;
@@ -49,7 +49,7 @@ pub const DEFAULT_BSO_TTL: u32 = 2_100_000_000;
 pub const FIRST_CUSTOM_COLLECTION_ID: i32 = 101;
 
 #[async_trait]
-pub trait DbPool: Sync + Send + Debug + GetPoolState {
+pub trait DbPool: Sync + Send + Debug + GetPoolStatus {
     type Error;
 
     async fn init(&mut self) -> Result<(), Self::Error> {

syncstorage-db-common/src/lib.rs

@@ -7,6 +7,7 @@ edition.workspace = true
 
 [dependencies]
 async-trait.workspace = true
+deadpool.workspace = true
 env_logger.workspace = true
 lazy_static.workspace = true
 log.workspace = true

syncstorage-db/Cargo.toml

@@ -29,7 +29,7 @@ pub use syncstorage_spanner::DbError;
 #[cfg(feature = "spanner")]
 pub type DbImpl = syncstorage_spanner::SpannerDb;
 
-pub use syncserver_db_common::{GetPoolState, PoolState};
+pub use syncserver_db_common::GetPoolStatus;
 pub use syncstorage_db_common::error::DbErrorIntrospect;
 
 pub use syncstorage_db_common::{

syncstorage-db/src/lib.rs

@@ -1,7 +1,7 @@
 //! Mock db implementation with methods stubbed to return default values.
 #![allow(clippy::new_without_default)]
 use async_trait::async_trait;
-use syncserver_db_common::{GetPoolState, PoolState};
+use syncserver_db_common::GetPoolStatus;
 #[cfg(debug_assertions)]
 use syncstorage_db_common::util::SyncTimestamp;
 use syncstorage_db_common::{BatchDb, Db, DbPool, params, results};
@@ -34,9 +34,14 @@ impl DbPool for MockDbPool {
     }
 }
 
-impl GetPoolState for MockDbPool {
-    fn state(&self) -> PoolState {
-        PoolState::default()
+impl GetPoolStatus for MockDbPool {
+    fn status(&self) -> deadpool::Status {
+        deadpool::Status {
+            max_size: 0,
+            size: 0,
+            available: 0,
+            waiting: 0,
+        }
     }
 }
 

syncstorage-db/src/mock.rs

@@ -20,7 +20,7 @@ use syncserver_common::{BlockingThreadpool, Metrics};
 #[cfg(debug_assertions)]
 use syncserver_db_common::test::test_transaction_hook;
 use syncserver_db_common::{
-    GetPoolState, PoolState, establish_connection_with_logging, manager_config_with_logging,
+    GetPoolStatus, establish_connection_with_logging, manager_config_with_logging,
     run_embedded_migrations,
 };
 use syncstorage_db_common::{Db, DbPool, STD_COLLS};
@@ -170,9 +170,9 @@ impl fmt::Debug for MysqlDbPool {
     }
 }
 
-impl GetPoolState for MysqlDbPool {
-    fn state(&self) -> PoolState {
-        self.pool.status().into()
+impl GetPoolStatus for MysqlDbPool {
+    fn status(&self) -> deadpool::Status {
+        self.pool.status()
     }
 }