Add support for Redis Sentinels

davidor commented 4 years ago

Closes #28

This PR adds support for Redis Sentinels (https://redis.io/topics/sentinel-clients)

Notice that this PR is not complete. It does not have tests, does not explain how to use this in the README, and possibly does not handle all the error cases, but I'd rather work on those things after we agree on other aspects first.

In order to test this, you'll need one Redis master, a replica, and 3 sentinels. These are the commands I run to setup this on my local machine using docker, but you can adapt them to your env.

Setup

Run master on port 6379

docker run --rm -it --net=host redis --port 6379

Run slave on port 6380

docker run --rm -it --net=host redis --port 6380

Configure the slave to follow the master

redis-cli -p 6380 slaveof 127.0.0.1 6379

Start 3 sentinels

I used this example config file from the official docs. You need to create one for each sentinel. The 3 files should be the same except for the port. I used 9000, 9001 and 9002.

port 9000
sentinel monitor mymaster 127.0.0.1 6379 2
sentinel down-after-milliseconds mymaster 5000
sentinel failover-timeout mymaster 60000
sentinel parallel-syncs mymaster 1

docker run --rm -it --net=host -v /tmp/sentinel1.conf:/data/sentinel.conf redis /data/sentinel.conf --port 9000 --sentinel

docker run --rm -it --net=host -v /tmp/sentinel2.conf:/data/sentinel.conf redis /data/sentinel.conf --port 9001 --sentinel

docker run --rm -it --net=host -v /tmp/sentinel3.conf:/data/sentinel.conf redis /data/sentinel.conf --port 9002 --sentinel

Test

Instantiate the client:

sentinels = [{host: '127.0.0.1', port:9000},{host:'127.0.0.1', port:9001},{host: '127.0.0.1', port: 9002}]
client = Async::Redis::SentinelsClient.new('mymaster', sentinels)

Make a request:
```
client.call('get', 'some_key')
```

Notice that the request should go to the master, and not the slave. You can verify that using the monitor Redis command.

To test a failover you can use: redis-cli -p 6379 debug sleep 10. That will block the master during some seconds, and the sentinels will elect a new master.

You can use something like this to verify that the client automatically sends requests to the new master when that happens. Again, you can run monitor on both Redis instances to verify this:

Async do
  loop do
    client.call('get', 'a')
    sleep 2
  end
end

davidor commented 4 years ago

@ioquatix I addressed your comments and added a new commit that handles an error case that I didn't take into account.

ioquatix commented 4 years ago

The error handling case makes sense.

This kind of distributed system has tricky error semantics.

The current implementation looks okay.

Actually, I tried to read the redis documentation, and I didn't understand what the hell this feature is. But I read the code, and it's crystal clear. Haha.

ioquatix commented 4 years ago

I'm pretty happy with this. I may play around with the composition vs inheritance thing, but I'm happy to merge. I wonder if we should try to test it some how?

davidor commented 4 years ago

@ioquatix I addressed your comments. Feel free to merge. As I mentioned above, changing to composition will require changes in the other class, so maybe it's better to address that separately? It should be fine as long as there isn't a release in the middle, because that would be a breaking change.

Regarding tests, testing with real Redis instances might be a bit too much for this, because we'd need 5 at least. On the other hand, we don't have any tooling to create "fake" async clients and I'm a bit concerned that using too much mocking will make the tests useless. Is there something in the async libraries that can help us with this?

ioquatix commented 4 years ago

There are some options, we could try mocking up fake redis servers.

You've done a great job bringing the code to this point. I didn't understand sentinels at all but it's really clear from the code.

davidor commented 4 years ago

@ioquatix about the composition vs inheritance topic. I implemented a possible solution in this WIP branch https://github.com/socketry/async-redis/compare/master...davidor:sentinels-change-to-composition but I'm not sure if it's a good idea to expose Client#pool. Is this what you had in mind?

ioquatix commented 4 years ago

It's a good idea but I think we need a bit more elegant solution.

ioquatix commented 1 week ago

FYI, I've added integration for Redis sentinels. Check the sentinel directory to see how it works.

socketry / async-redis

Add support for Redis Sentinels #29

Setup

Test