Replace AsyncConnectionConfig with Manager builder pattern

[bombsimon]

Removes Manager::new_with_config which exposed internal redis settings. Adds Manager::builder() with connection_timeout() and response_timeout() setters that only surface user-facing options.

---

Followup from #474. Thanks for the quick turnaround and sorry for confusing you and just exposing the whole config.

Since this now adds two new settings to the Manager and there's already a builder pattern for the Config I opted to use the builder pattern here as well. LMK if you rather have a separate new_with_timeouts constructor or something instead.

[bikeshedder]

That's much better. Though I'm seriously wondering if we could add a proper configuration object instead. What I mean by that is the following: Almost all deadpool-* crates maintained by me try to offer a ready-to-use configuration object that can be dropped into your own configuration. That makes the usage in actual applications much simpler as you don't need to worry about defining your own configuration structure for database connectivity.

https://github.com/deadpool-rs/deadpool/blob/main/crates/deadpool-redis/src/config.rs

For that reason all config structures in deadpool are serde (de)serializable.

[bombsimon]

Though I'm seriously wondering if we could add a proper configuration object instead.

SGTM! I updated the code to now put the timeouts on the existing Config struct. I added to with_ setters similar to sentinel::Config.

With this I now dropped the builder for Manager and our config will build it instead. A result is that you can't build a Manager directly with timeouts without going via the Config. I'm not sure this is an issue? If so, should I create a new path where we can build a manager by passing timeouts and/or just use that one instead of build_manager on Config? I avoided it because it doesn't scale that well adding new constructors for every new config permutation.

[bikeshedder]

Just a few nitpicks. Option<Option<Duration>> really isn't great DX and I think we should expose new_with_config as public method.

[bikeshedder]

Now that I think about it I think we should keep that method public. Otherwise users can only create a manager with timeouts using the config way.

I'm unsure if we should force users to always use the config code path. Even though I find that much more ergonomic than fiddling with the redis structures directly.

[bombsimon]

Yeah that was my concern as well above and I agree it's not ideal. What do you want this method to accept? Two durations? A struct containing them? A builder pattern?

[bikeshedder]

I see your point. The AsyncConnectionConfig is essentially a "write-only" structure. I hate this pattern and I don't get why so many libraries do that. 😫

I think there are basically two options. Roll our own structure - which is basically what you did at first - or accept that this is essentially something opaque passed by the user and we have no control over it. We can't even validate it.

The only real trouble maker I see in this struct is set_push_sender which deadpool will have to override in the future in order to support RESP3.

[bombsimon]

I think a builder pattern (the ManagerBuilder) makes most sense because it's forward compatible and will allow you to add new config options without breaking backwards compatibility.

Then also having the values on the Config, defaulting to the same as redis, and pass them to the manager builder pattern.

[bombsimon]

Sorry if this grew, especially now with the AsyncConnectionConfig on main which isn't ideal. Should we put a pin in this until a more generic approach is reached? If so, should I revert the previous change so releasing main isn't blocked?

One thing with #[non_exhaustive] that surfaced with the doc test that I forgot about, it's not possible to build them externally.

Internally I can write a struct literal like this:

ManagerConfig {
    connection_timeout: Some(Duration::from_secs(5)),
    response_timeout: None,
    ..Default::default()
}

But externally the user would have to write this:

let mut config = ManagerConfig::default();
config.connection_timeout = Some(Duration::from_secs(5));
config.response_timeout = None;

Reference to why.

I personally don't love that pattern so that could be one argument for a builder pattern I guess.

[bikeshedder]

Ideally you would never need to hardcode this in your application anyways. Overwritten defaults, yes, maybe. But in an actual application you would probably want to have a VALKEY__CONNECTION_TIMEOUT=2s environment variable (or some sort of configuration file).

I've written a rationale here:

• #481

Not being able to spread ..Default::default() is quite a bummer. I was a bit curious why that's the case and got the answer in the document you linked:

We’d expect this code to work without the non_exhaustive attribute:

let c = Config { width: 1920, height: 1080, ..Config::default() };
println!("{:?}", c);

Although outside of the defining crate, it will not, because Config could, in the future, contain private fields that the user didn’t account for.

It's too late for me and I seriously don't care THAT much. I just want the deadpool-* crates to be consistent. e.g. in deadpool-postgres I didn't even bother adding #[non_exhaustive] to the config struct. If I ever need to add a new configuration variable it's going to be a breaking change. But one that's easy to fix. Just bump the version number and see if your code still compiles. If you were matching the config it's probably for the better that it's breaking because you might have missed a config variable that you are now interested in.

If you want to call it the day just make it a plain struct without the #[non_exhaustive]. That'd be fine for me as it has the best DX in my opinion.

[bikeshedder]

fyi. I've spent the past hour debating this with myself ... I see it like that:

• When adding #[non_exhaustive] to a config struct a builder pattern should be added for convenience. Though I think the fields should stay public for easier access (and being able to match). I despise setters and getters for config structs.
• Leaving out #[non_exhaustive] will introduce breaking changes when new fields are added. The structs are fully (de)serializable and adding a fields does change the representation. Maybe it is a good thing that this is a breaking change?

[bikeshedder]

Let's get this merged. Just drop the #[non_exhaustive] so it's on-par with the other deadpool-* config structures. I'm fine with that solution. It's good enough. 😅

[bombsimon]

Sounds good! I think just fixing compiler issues and adding defaults on breakage on new fields is reasonable for now. It's likely not bound to happen that often.

Really appreciate your time on this and the discussion! I've pushed removal of #[non_exhaustive] but am happy to continue discussion here or linked issue.

[bikeshedder]

There is an interesting split now. There is a deadpool_redis::Config structure which comes with a create_pool method. Setting the new connection and request timeouts is only possible if you construct the manager directly and not via the Config structure.

I guess the most correct approach here would be to add the timeout fields to the Config struct as well and construct the Manager via the new Manager::new_with_config method.

Also I just now realized that the new ManagerConfig doesn't really need to be (de)serializable as it's not used as part of the Config struct. It is not wrong to add serde support here, too, but it's really not necessary and probably better to remove it so being Deserialize and Serialize doesn't become public API.

I'm so sorry for mixing that up. 🤦

[bombsimon]

I guess the most correct approach here would be to add the timeout fields to the Config struct as well and construct the Manager via the new Manager::new_with_config method.

The timeout fields are on the Config here and the manager is built with new_with_config here.

Also I just now realized that the new ManagerConfig doesn't really need to be (de)serializable as it's not used as part of the Config struct. It is not wrong to add serde support here, too, but it's really not necessary and probably better to remove it so being Deserialize and Serialize doesn't become public API.

There's no serialization macro on ManagerConfig from what I can see from here

Maybe I'm misunderstanding both comments? 😅