Iam

[emcee21]

No description provided.

[bikeshedder]

I superficially reviewed it and there is one thing that I really don't like about it:

Rather than adding new methods to the ManagerConfig and adding new fields to the ManagerConfig I would prefer if this was designed more like a plug-in. I'd like to see a ConfigProvider trait that can then be made as AWS specific as necessary:

e.g.

pub trait ConfigProvider {
    async fn get_config(&self) -> Config;
}

I'm really just sketching this in my head right now.

Maybe Config should be made a trait and BasicConfig and AwsConfig could then implement this new trait?

If connections need to be discarded after a given time that could be done via a pre_recycle hook.

[emcee21]

Makes sense. Fwiw I thought the “max_age” feature might have applicability beyond AWS.

…

On Mar 19, 2025, at 11:31 AM, Michael P. Jung ***@***.***> wrote: bikeshedder left a comment (deadpool-rs/deadpool#398) I superficially reviewed it and there is one thing that I really don't like about it: Rather than adding new methods to the ManagerConfig and adding new fields to the ManagerConfig I would prefer if this was designed more like a plug-in. I'd like to see a ConfigProvider trait that can then be made as AWS specific as necessary: e.g. pub trait ConfigProvider { async fn get_config(&self) -> Config; } I'm really just sketching this in my head right now. Maybe Config should be made a trait and BasicConfig and AwsConfig could then implement this new trait? If connections need to be discarded after a given time that could be done via a pre_recycle hook. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread. <#398 (comment)> <https://github.com/notifications/unsubscribe-auth/AAAMXUX3ZLMODFNQKKTLPKL2VGZYVAVCNFSM6AAAAABZLQXAS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMZXGY2DQOJXGE> bikeshedder left a comment (deadpool-rs/deadpool#398) <#398 (comment)> I superficially reviewed it and there is one thing that I really don't like about it: Rather than adding new methods to the ManagerConfig and adding new fields to the ManagerConfig I would prefer if this was designed more like a plug-in. I'd like to see a ConfigProvider trait that can then be made as AWS specific as necessary: e.g. pub trait ConfigProvider { async fn get_config(&self) -> Config; } I'm really just sketching this in my head right now. Maybe Config should be made a trait and BasicConfig and AwsConfig could then implement this new trait? If connections need to be discarded after a given time that could be done via a pre_recycle hook. — Reply to this email directly, view it on GitHub <#398 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMXUX3ZLMODFNQKKTLPKL2VGZYVAVCNFSM6AAAAABZLQXAS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMZXGY2DQOJXGE>. You are receiving this because you authored the thread.

[bikeshedder]

The max_age feature is something that can be of interest for every deadpool-* crate. There are some examples how to write that max age hook in the repo. It really needs some better documentation though.

[bikeshedder]

Found the example:

• Feature Request: Support max_lifetime. #359 (comment)

Pool::builder(...)
    // ...
    .pre_recycle(Hook::sync_fn(|_, metrics| {
        if metrics.age() > max_age {
            Err(HookError::message("Max age reached"))
        } else {
            Ok(())
        }
    }))
    // ...

[bikeshedder]

It'd be nice if this felt more like a plugin. I've been toying with the idea of plugins some time ago. The API would then look like that:

Pool::builder(...)
    .add_plugin(MaxAge(Duration::from_seconds(300))

There are some unanswered questions though:

• How does one change the configuration of a plugin?
• How to add and remove plugins while the pool is running?
• The order of plugins does matter! (Not a question but a finding while working on that idea)

Those were the reasons why I think making the entire pool hot swappable is the way to go in the future. This would allow maximum flexibility and enable basically every nice use case that I can think of.

---

The auth credentials are a bit different though. They don't require a pool reconfiguration and swapping of the manager. I can imagine some provider using OTPs that need to change for every single connection. So adding a way to dynamically create the configuration as needed makes a lot of sense.

[bikeshedder]

Sorry for using this PR as a rubber duck for my thoughts. I'll create a separate "meta" issue and update the milestones to reflect those ideas.

I understand that you need this feature asap and you don't want to maintain a separate branch for too long. Just give me a day or two to finally make up my mind on that topic. I really think it's time to improve this part for all deadpool-* crates once and for all.

[emcee21]

No worries at all! My plan is to keep debugging the current implementation. As you gather your thoughts I can come back to implement this PR as a proper candidate.

…

On Mar 19, 2025, at 12:39 PM, Michael P. Jung ***@***.***> wrote: bikeshedder left a comment (deadpool-rs/deadpool#398) Sorry for using this PR as a rubber duck for my thoughts. I'll create a separate "meta" issue and update the milestones to reflect those ideas. I understand that you need this feature asap and you don't want to maintain a separate branch for too long. Just give me a day or two to finally make up my mind on that topic. I really think it's time to improve this part for all deadpool-* crates once and for all. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread. <#398 (comment)> <https://github.com/notifications/unsubscribe-auth/AAAMXUWIAQILHT73V5IMNET2VHBYRAVCNFSM6AAAAABZLQXAS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMZXHA4DINBUG4> bikeshedder left a comment (deadpool-rs/deadpool#398) <#398 (comment)> Sorry for using this PR as a rubber duck for my thoughts. I'll create a separate "meta" issue and update the milestones to reflect those ideas. I understand that you need this feature asap and you don't want to maintain a separate branch for too long. Just give me a day or two to finally make up my mind on that topic. I really think it's time to improve this part for all deadpool-* crates once and for all. — Reply to this email directly, view it on GitHub <#398 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMXUWIAQILHT73V5IMNET2VHBYRAVCNFSM6AAAAABZLQXAS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMZXHA4DINBUG4>. You are receiving this because you authored the thread.

[bikeshedder]

• Add support for dynamic postgres connection config #219