Backwards incompatible changes to consider

[nihohit]

The purpose of this list is to raise major design decisions which we should consider for a major backwards incompatible version, such as 1.0. Everyone is welcome to comment on these, or offer more suggestions.

Connections:

• Reduce ConnectionInfo's visibility - this hampers our ability to add fields with structs that we don't want to be public. We could expose a builder, instead, or rely on redis URLs. Sample issue - TlsConnParams is opaque: Intentional? #1006
• async connections that work with shared references -
  Blocking operations and aio MultiplexedConnection #1236 (comment)

Parsing and values:

• Make from_redis_value take the value, instead of a reference. Today we have to maintain both from_redis_value and from_owned_redis_value. We should remove from_owned_redis_value, have from_redis_value take an owned value. and reduce the duplication. If the user wants to convert by reference, they can just clone the value.
• Similarly, redis::Value can contain better clonable values - replace String with Arc<String> or ArcStr, or maybe just Bytes objects. This will reduce the cost of cloning values, and if we replace String and Vec<u8> with Bytes it can even allow for zero-copy parsing.
• Similarly, consider using redis-protocol for parsing, value. It looks like it allows for zero-copy parsing already.

Async

• Currently the library supports both async-std and tokio as runtimes, and allows compilation with both features. We should consider whether we want to go with full runtime agnostic and also support smol.
• Alternatively, if there's no adoption of async-std, we can drop that support and reduce the async implementation complexity.
• Orthogonally, we should consider whether we allow compiling with both the async-std and tokio features, or whether these features should be mutually exclusive.

[kamulos]

Some things we encountered in the past:

• For Connections like MultiplexedConnection or cluster_async::ClusterConnection it would be great if executing a command took &self instead of &mut self. They should have interior mutability anyway (?). This was previously discussed in Why Connection should be &mut rather than & when execute commands? #638 .

• There is no good way to handle pipelines where some of the commands succeed and others fail. This was previously discussed in Add Value::Error and some compatibility functions #813 and There should be an option to process the results of pipeline batch on a caller side. #746 .

• If I pass parameters to a script where some of the parameters might be None they are just skipped and the following parameters jump a place to the front. Ultimately this is a limitation of the RESP2 protocol, I guess, but the behavior of redis-rs in this case is very surprising and easily leads to mistakes.

• In Async Cluster: AWS Failover takes a long time to recover #1005 the idea was floated to introduce some sane default timeouts in connections, so everyone that does not explicitly set timeouts will get a good experience.

[nihohit]

Thanks!

If I pass parameters to a script where some of the parameters might be None they are just skipped and the following parameters jump a place to the front. Ultimately this is a limitation of the RESP2 protocol, I guess, but the behavior of redis-rs in this case is very surprising and easily leads to mistakes.

I was unaware of this issue. Can you give me a code sample (preferably one that works in redis-rs tests) that shows the issue?

to introduce some sane default timeouts in connections

How would you go about choosing the correct values?

[kamulos]

I was unaware of this issue. Can you give me a code sample (preferably one that works in redis-rs tests) that shows the issue?

This is the most minimal example I could build:

use redis::{Connection, Script};

fn main() {
    let client = redis::Client::open("redis://127.0.0.1/").unwrap();
    let mut con = client.get_connection().unwrap();

    println!("some arg: {}", execute_script(&mut con, Some("first"), "second"));
    println!("none arg: {}", execute_script(&mut con, None, "second"));
}

fn execute_script(con: &mut Connection, first: Option<&str>, second: &str) -> String {
    Script::new(RETURN_FIRST_ARG).arg(first).arg(second).invoke(con).unwrap()
}

const RETURN_FIRST_ARG: &str = r#"
return redis.status_reply(ARGV[1])
"#;

It will print out:

some arg: first
none arg: second

How would you go about choosing the correct values?

My intuition would be to have a conservative / big value, that's likely too big for most people. I think Redis is mostly chosen for speed and latency, so network latency should usually be low. Long running scripts have a default timeout of 5s on the server side. But having the Redis block for 5s is crazy for most applications.

For the connection timeout there is a precendence of 1s in the async cluster. For the request timeout maybe something in the range of 1 to 5 seconds?

[jaymell]

For the connection timeout there is a precendence of 1s in the async cluster. For the request timeout maybe something in the range of 1 to 5 seconds?

I think the problem with request timeouts specifically is that a lot of Redis commands explicitly block. I think it would make for a bad experience if users don't realize there's a default request timeout in place breaking their properly constructed commands.

[nihohit]

Thanks for the example!
I need to run this locally and fully understand what happens here, but looks like the issue is that Option implements ToRedisArgs

redis-rs/redis/src/types.rs

Line 1084 in cc32c77

impl<T: ToRedisArgs> ToRedisArgs for Option<T> {

Which doesn't make sense to me. I can see how it saves some repeating lines of code, but as the example shows, it reduces actual legibility of the code - arg is called without adding an arg.
By the same token, just like args that are resolved to 0 args shouldn't be allowed, maybe multiple arguments, e.g. Vec should also be added as args and not arg?

[altanozlu]

• Similarly, consider using redis-protocol for parsing, value. It looks like it allows for zero-copy parsing already.

https://crates.io/crates/redis-protocol is a nice crate, maybe we should benchmark it.

• For Connections like MultiplexedConnection or cluster_async::ClusterConnection it would be great if executing a command took &self instead of &mut self. They should have interior mutability anyway (?). This was previously discussed in Why Connection should be &mut rather than & when execute commands? #638 .

Since aio will be gone, aio's ConnectionLike can be changed.

• There is no good way to handle pipelines where some of the commands succeed and others fail. This was previously discussed in Add Value::Error and some compatibility functions #813 and There should be an option to process the results of pipeline batch on a caller side. #746 .

We could work on them.

[aembke]

Let me know if there's any changes or new features you'd like to see with redis-protocol. For what it's worth only the decode-mut interface supports zero-copy parsing at the moment, but it wouldn't be too difficult to apply the same strategy to the more generic byte slice parsing interface. But as @nihohit noted, if you switch to Bytes types for the dynamically sized Value variants then the decode-mut interface that works with BytesMut buffers will likely be a better fit since it uses Bytes and Str internally.

[nihohit]

@aembke Thanks!
From a brief look, the only thing I see missing from redis-protocol is an equivalent to the FromRedisValue trait, that allows easy conversion between Redis types and regular types.
The main issue that currently makes me wait with trying it is backwards compatibility. I don't see a way that doesn't include a lot of glue code to use the crate without it being a massive break for users. It's bad enough to change an out Vec to Bytes, it's quite worse to change the names of the value outputs.

[nihohit]

Re: error handling:
#1056
#1057

two proposals for better error handling in pipelines.

[kamulos]

One more thing that confused me today is the hget function, that mixes the Redis hget and hmget commands. When I fetch a single field using hmget, Redis will return an array containing one string. But this is not mirrored by redis-rs and it will return Value::Data instead of Value::Bulk.

At first glance combining those two commands seems convenient, but I think this might be worth a breaking change to better represent the behavior of Redis, and cause less surprises.

The redis-rs code for reference:

    fn hget<K: ToRedisArgs, F: ToRedisArgs>(key: K, field: F) {
        cmd(if field.is_single_arg() { "HGET" } else { "HMGET" }).arg(key).arg(field)
    }

[altanozlu]

I don't think async std is maintained anymore https://github.com/async-rs/async-std/graphs/code-frequency we can drop it.
Maybe instead of having smol we can provide an example using it with https://docs.rs/async-compat/latest/async_compat

[jaymell]

One more thing that confused me today is the hget function, that mixes the Redis hget and hmget commands.

I agree and think we should also simplify GET, which uses a similar approach.

[altanozlu]

renaming ConnectionInfo into RedisConfiguration and making it general would be better, for example client side caching information, resp3, push message callback.

[altanozlu]

how about changing aio::ConnectionLike to use async fn instead of returning RedisFuture ? Ofc we need to bump MSRV.

[dcamsiteimprove]

If I can add another suggestion, I think there should be some separation of which commands are available in cluster connections and normal connections.

Currently the ClusterConnection exposes some methods that operate only on individual nodes, like scan() (and possibly others), which don't make any sense to call on a cluster connection, but only on connections to individual servers.
In our case, we had an issue with scan() that went undetected because the behavior of CluterConnection is to connect to a random node in the cluster, and that node might be different from the node specified in the constructor of the ClusterClient.

Our code looked like this, which looks fine at a glance but in practice sends the scan to a random node every time, duplicating some values and skipping some others:

    for node_url in redis_urls {
        let client = ClusterClient::builder([node_url])
            .read_from_replicas()
            .build()?;
        let reader_conn = client.get_async_connection().await?;
        let scan_iterator = reader_conn.scan().await?;

I think splitting the ConnectionLike into 3 traits would be nice to make these usage errors less likely:

• ServerConnectionLike -> scan(), info() or anything node-specific. NOT implemented by ClusterConnection. Can maybe be reused for PubSub?
• ConnectionLike -> get(), set(), and most of the commands. The commands will be switching nodes automatically if needed, and will have the expected behavior on both Connections and ClusterConnections. It can be implemented by both types.
• ClusterConnectionLike -> get cluster info / get the list of nodes/slots out out of the connection client? I'm not sure it's really needed as these commands would work also on a node connection. NOT implemented by Connection.

[nihohit]

@dcamsiteimprove that's pretty similar to what we did in GLIDE for Redis. As an interim solution, you can use the route_command method on cluster connections in order to send commands and specify to which node the command should be sent.

[nihohit]

async connections that work with shared references -
#1236 (comment)