Sentinel

[prk3]

This PR is far from being ready for merge. Its purpose it to show how we might make the library more flexible and add support for Sentinel. Check out issue 67 for the background.

This is a pretty big change to the internals of the library. Public API has also changed, although mostly in terms of types - I wanted to keep the feel of the original.

So, what has changed?

Builders

Previously, there was only one builder: ConnectionBuilder. It did not hold data necessary for connecting to Redis through sentinel (redis master name, sentinels address list, credentials). Now there are two builders: RedisConnectionBuilder and SentinelConnectionBuilder. They have different fields and methods, but implement the same trait: ConnectionBuilder. ConnectionBuilder trait requires one method, connect, which establishes a primitive (RespConnection) connection to Redis. While RedisConnectionBuilder does nothing fancy (simply calls the old connect_with_auth function), SentinelConnectionBuilder asks all sentinels for Redis master address and connects to it after double checking Redis role.

Similarly to how it worked before, any type implementing ConnectionBuilder can start paired or pubsub connection by calling paired_connect or pubsub_connect on the builder. The only difference is that now users will have to add an appropriate trait to the scope, that is either client::paired::PairedConnectionBuilder or client::pubsub::PubsubConnectionBuilder. Internally, these methods create primitive connection and upgrade it to a complex connection (Paired/Pubsub).

Reconnecting

Until now, one couldn't opt out of reconnecting in PairedConnection and PubsubConnection. That is not the case anymore. Both connection types can be created from RespConnection and error channel. More on that in a second. I've added Reconnect struct holding builder and connection state. Reconnect is generic over ConnectionBuilder and ComplexConnection. ComplexConnection is a trait with pretty loose requirements: implemeneters must be able to construct themselves from RespConnection and error channel. Error channel is a oneshot sender that informs Reconnect struct that the connection should be recreated (using the builder). Users can access the currently active complex connection by calling .current on Reconnecting. Because we make few assumptions about the complex connection, it's easy to use Reconnect with your own code. For convenience, PairedConnection adds send and send_and_forget to Reconnect holding PairedConnection. PubsubConnection could do the same.

Side effects

I made functions/methods like paired_connect return non-reconnecting connections and added paired_reconnecting. People migrating to a new version might not notice that. Maybe we should keep old functions as they were and add paired_non_reconneting instead?

Considerations

Is error channel a good way of informing Reconnect about errors? Maybe we should wrap RespConnection with some struct that spies on the tcp channel and and pass that to complex connection? There might be a use case for having that level control in complex connection.

Maybe Reconnect should not be generic over ConnectionBuilder but instead store Box<dyn ConnectionBuilder>? This way we wouldn't have to mention the builder type when describing the type of a Connection, e.g.

use redis_async::client::{Reconnecting, PairedConnection, builder::redis::RedisConnectionBuilder};

struct AppState {
    pub redis: Reconneecting<RedisConnectionBuilder, PairedConnection>,
}

vs

use redis_async::client::{Reconnecting, PairedConnection};

struct AppState {
    pub redis: Reconneecting<PairedConnection>,
}

I think we can afford that. We'd possibly have to relax trait bounds.

What do you think of this design?

[prk3]

I forgot to add. Here is how you'd use the API:

// direct connection, paired, reconnecting

use redis_async::client::{RedisConnectionBuilder, paired::PairedConnectionBuilder};

let client = RedisConnectionBuilder::new("url")
    .username("hello")
    .password("world")
    .paired_reconnecting()
    .await
    .unwrap();

let response = client.send(data).await.unwrap();

// connecting through sentinel, pubsub, not reconnecting

use redis_async::client::{SentinelConnectionBuilder, pubsub::PubsubConnectionBuilder};

let client = SentinelConnectionBuilder::new(vec!["host 1", "host 2"], "master_name")
    .redis_username("foo")
    .redis_password("bar")
    .pubsub_connect()
    .await
    .unwrap();

let stream = client.subscribe("message").await.unwrap();

[benashford]

Hello,

Thanks for this 👍 This is quite a big PR so I'll some more comments later once I have the chance to go in to some of the details in a bit more detail.

But some high-level comments:

Regarding reconnection - I think I would prefer if the default path maintained backward compatibility, just to reduce the risk of users not noticing the modified behaviour. But I could be persuaded otherwise if non-reconnecting clients was the idiom, but I think most Redis clients I've used for other programming languages have automatically handled reconnection. So on balance, I think that should probably be the default and non-reconnecting made the option.

I do like the new reconnection logic. It's much cleaner than what was there before, which grew organically for several versions and is currently quite gnarly.

One thing I'm not sure about how the library should behave in future is the send method on PairedConnect. Currently it's implemented in such a way so that the precise ordering of messages sent to Redis are the same as the order that send is called. This goes against the Rust idioms of futures doing nothing unless they're polled, but it did have two benefits: One, it meant that there wouldn't be any surprises if a future wasn't polled for whatever reason, so this would still work, even though the first future is thrown away without being .awaited on. It means we can also .await them in a different order if it makes sense to or even accidentally.

let set_future = con.send(resp_array!["SET", "x", "12"]);
let get_result = con.send(resp_array!["GET", "x"]).await;

Secondly, it enabled the implicit pipelining to occur (as described in the project's README). So I'm not sure where we should go with this. On the one hand, by making send a more classically async function we have something that's more idiomatic, but it could result in unsuspecting performance regressions in users of the library. So I need to have a bit of a think. This has been an issue for a while, but changes required for this PR might force the issue. In order to limit the size of this PR it might be something I need to fix in a separate PR which leapfrogs this one (possibly, I still need to think through the consequences).

Is error channel a good way of informing Reconnect about errors? Maybe we should wrap RespConnection with some struct that spies on the tcp channel and and pass that to complex connection? There might be a use case for having that level control in complex connection.

My first thought for this one is it should be fail-safe in the sense that we don't want it to be possible for the state the be NotConnected without a reconnection attempt being triggered. It looks like this is the case, we should see errors if anything went wrong.

I'll check out the PR locally in more detail over the next few days as time allows.

Thanks again 👍

[prk3]

I think I would prefer if the default path maintained backward compatibility, just to reduce the risk of users not noticing the modified behaviour. But I could be persuaded otherwise if non-reconnecting clients was the idiom, but I think most Redis clients I've used for other programming languages have automatically handled reconnection. So on balance, I think that should probably be the default and non-reconnecting made the option.

Agree. Let's not confuse people.

Secondly, it enabled the implicit pipelining to occur (as described in the project's README).

Do you mean sending multiple messages without waiting for responses? Like this?

client        redis

request A >
request B >
request C >

some time passes...

       < response A
       < response B
       < response C

I believe this still works, but you have to fire multiple futures at once. It might be even more intuitive, if you compare it to sleep for example:

let sleep_1 = tokio::time::sleep(Duration::from_secs(1));
let sleep_2 = tokio::time::sleep(Duration::from_secs(2));
let sleep_3 = tokio::time::sleep(Duration::from_secs(3));

sleep_1.await; // takes 1 second
sleep_2.await; // takes 2 seconds
sleep_3.await; // takes 3 seconds
               // 6 seconds in total 

// vs

let sleeps = vec![
    tokio::time::sleep(Duration::from_secs(1));
    tokio::time::sleep(Duration::from_secs(2));
    tokio::time::sleep(Duration::from_secs(3));
];

let _ = future::join_all(sleeps).await; // takes 3 seconds

My first thought for this one is it should be fail-safe in the sense that we don't want it to be possible for the state the be NotConnected without a reconnection attempt being triggered. It looks like this is the case, we should see errors if anything went wrong.

My thinking here was that we'll probably want to detect disconnects in paired connect anyway to cancel/fail request futures. And if we're doing that, it would be really easy to notify reconnect. In addition, complex connections could trigger reconnect if they find themselves in some unrecoverable state.