bikeshedder / deadpool

Dead simple pool implementation for rust with async-await
Apache License 2.0
1.06k stars 138 forks source link

Split PoolConfig and ConnectionConfig into separate structures #74

Open bikeshedder opened 3 years ago

bikeshedder commented 3 years ago

Right now the Config structs of the deadpool-* crates contain all the connection configuration and a pool: PoolConfig field. It would be better to move the configuration specific to the connection/manager into its own structure.

Right now the Config structs implements a create_pool method which creates a manager using the connection/manager specific fields and uses the pool field to create the actual Pool.

I think a better design would be to have a PoolConfig and ManagerConfig structure which are both part of a new structure called Config. The new Config structure would look more or less like that:

#[derive(Clone, Debug)]
#[cfg_attr(feature = "config", derive(serde::Deserialize))]
struct Config {
    pub manager: Option<ManagerConfig>,
    pub pool: Option<PoolConfig> 
}

impl Config {
    pub fn create_pool(self) -> Pool {
        let manager = crate::Manager::new(self.manager);
        Pool::from_config(manager, self.pool);
    }
}

The create_pool would now consume self and also enable the implementation of the Into/From traits:

impl From<Config> for Pool {
    fn from(config: Config) -> Self {
        config.create_pool()
    }
}

The Config struct could even be made into a generic Config<M> structure, but I'm undecided if that's actually worth it.

bikeshedder commented 3 years ago

The only downside with this I see right now is that writing the configuration like that would require the the configuration parameters to be nested within a manager property:

{
    "pg": {
       "manager": {
           "host": "/run/postgresql"
       },
       "pool": {
           "max_size": 64
       }
    }
}

It should be possible to keep the old configuration structure using some serde annotations, but feel like this should be avoided in the interest of clarity.

gyfis commented 3 years ago

@bikeshedder Thanks a lot for opening the issue! My two cents (from a Rust beginner :)):

I don't see the added value of Into/From traits for Config <-> Pool; it seems calling create_pool that consumes the config is already very explicit and simple.

I also don't really dig the idea of extracting the connection details into a specific config sub-field; even though the structure is a bit more clear, the usability is lower.

What do you think about flattening the deadpool-* Config structs to include the max_size and timeout_* attributes directly in the struct, with respective defaults next to the connection details, like this?

e.g. Redis (Postgres config is a bit longer currently)

pub struct Config {
  pub url: Option<String>,
  pub pool_size: Option<usize>,
  pub timeout_wait: Option<Duration>,
  pub timeout_create: Option<Duration>,
  pub timeout_recycle: Option<Duration>,
}

Internally, the conversion from these specific fields to the PoolConfig/Timeout could still happen, possibly by extracting the PoolConfig behavior to a Trait of sorts...