cartesi / rollups

Cartesi Rollups
30 stars 12 forks source link

Redis clients should connect to cluster #12

Closed gligneul closed 1 year ago

gligneul commented 1 year ago

📚 Context

We might need to connect to a cluster when using a hosted Redis. When connecting to a Redis cluster, the client can receive -ASK and -MOVED responses informing which node in the cluster to send the commands to.

✔️ Solution

Additional to the --redis-endpoint parameter, we should have a --redis-cluster (or any other better suggestion). It is worth taking a look at what redis-cli does.

📈 Subtasks

endersonmaia commented 1 year ago

I'm thinking about the CLI arguments naming:

I prefer the first option and assume the endpoint will be a well-configured redis cluster that will communicate with the client and inform the cluster node to connect.

torives commented 1 year ago

Unfortunately, the client's method of connecting to a cluster is different from the standard one. This forces us to know in advance if we're talking to a cluster or not. Is there a way to ask the server if it is in a cluster? Otherwise, we would probably need another parameter or a list of endpoints to differentiate.

What worries me about the --redis-endpoint option is that when I connected to a local cluster with a single endpoint and it failed to respond, the client's connection to the cluster was lost. It might be the case that my local cluster wasn't well-configured, and/or I'm lacking some piece of knowledge here.

torives commented 1 year ago

Is there a way to ask the server if it is in a cluster?

The CLUSTER SHARDS command might do the trick if we're willing to go this route.

gligneul commented 1 year ago

We could have two arguments:

One and only one of these arguments should be set.

endersonmaia commented 1 year ago

Sharing a snippet of the redis-cli --help output:

Usage: redis-cli [OPTIONS] [cmd [arg [arg ...]]]
  -h <hostname>      Server hostname (default: 127.0.0.1).
  -p <port>          Server port (default: 6379).
  -s <socket>        Server socket (overrides hostname and port).
...
  -u <uri>           Server URI.
...

There's no indication if the server is a cluster or not.

endersonmaia commented 1 year ago

See:

Also, from https://redis.io/docs/reference/cluster-spec/

... clients may be redirected to other nodes using redirection errors -MOVED and -ASK. The client is in theory free to send requests to all the nodes in the cluster, getting redirected if needed, so the client is not required to hold the state of the cluster. However clients that are able to cache the map between keys and nodes can improve the performance in a sensible way.

endersonmaia commented 1 year ago
* `--redis-cluster-endpoints` for a list of endpoints in a cluster.

This is weird, the client shouldn't need to know the available nodes, the cluster manager could add more nodes or remove others without having to communicate.

And as I said before, having to inform many hostnames would transfer the responsibility to choose where to connect to the client.

I think the client should be smart to receive only a single endpoint and discover if it's a cluster or not.

gligneul commented 1 year ago

Sharing a snippet of the redis-cli --help output: There's no indication if the server is a cluster or not.

If you want to connect to a cluster with redis-cli you need to pass the -c arg. See https://developer.redis.com/operate/redis-at-scale/scalability/redis-cli-with-redis-cluster/

This is weird, the client shouldn't need to know the available nodes, the cluster manager could add more nodes or remove others without having to communicate.

The client won't need to inform all endpoints. If he passes a list with a single node shard, the node will automatically discover the others. However, it should be optional to pass all the cluster nodes.

Here is the API that we will be using to connect to a cluster: https://docs.rs/redis/latest/redis/cluster/struct.ClusterClient.html#method.new

I think the client should be smart to receive only a single endpoint and discover if it's a cluster or not.

This is not easy. Even redis-cli doesn't do that; it requires the -c option. I'd rather keep the node code simpler and receive a parameter to indicate whether the node is connected to a cluster.

endersonmaia commented 1 year ago

I prefer following the redis-cli flag option, and maybe add a --enable-redis-cluster=bool or --redis-cluster-enabled=bool or something, and keep only one option to define the endpoint, that supports a lists, like --redis-endpoints=[]

torives commented 1 year ago

Considering that this is more of a cosmetic choice if you don't mind, I'd like to keep it as it is: with the two separate endpoint parameters. We're not exactly mimicking the redis-cli API anyway (it doesn't handle multiple endpoints) and with the added benefit of not having to change the already implemented code or the rollups examples because of this 😉