Potentially ignoring all or certain redis command errors

[film42]

Hey friends! Long time user of this gem and we recently had to move our redis sentinel setup over to redis cluster. Since redis-rb has no support for redis cluster, we ended up using corvus to proxy between redis-rb and redis cluster. In the past :raise_errors has been great for preventing requests from failing during a redis blip, but now that we're using a proxy, we get a new Redis::CommandError of ERR Proxy error from corvus if redis cluster is down. If you're open to allowing me to adding support for this problem in this gem, I think 1 of the 2 approaches would work:

1. Rescue Redis::CommandErrors and only raise if :raise_errors is disabled. This is an easy change but has negative side effects since a Redis::CommandError is something that should probably not be swallowed.
2. Add a whitelist of Redis::CommandErrors that can safely be ignored as an argument. By default, no errors are ignored, but you could do something like:

{ :raise_errors => false, :ignored_command_errors => ["ERR Proxy error"] }

Of the two options, I think (2) is far more maintainable. I have a branch for (2) ready to open as a PR, but I wanted to check to see if this is something you'd be interested in accepting. Thanks!

[tubbo]

@film42 While I understand this solves the problem of that particular error, I think the problem is actually upstream: Redis.rb doesn't support redis-cluster. I feel as though the effort should be made to integrate the concepts of antirez/redis-rb-cluster into redis/redis-rb, rather than try to fix the problem downstream here in redis-store.

In redis/redis-rb#546 (comment), @badboy indicates that a pull request for this feature would be considered, but they aren't going to devote any resources on it. Since Redis::Store subclasses Redis, we'll take on the changes if the main Redis client supports them.

[film42]

Thanks for the reply @tubbo . I actually think this is the place to add the code since this gem is already rescuing IO and specific redis connection errors.

I've spoken with the redis-rb folks on IRC and I've read through that github issue as well. For them it's not about the code, but a maintainer for redis cluster support. A few of the redis-rb maintainers have actually started using corvus as a proxy, so we followed their example. In fact, @badboy has contributed to the corvus project, interestingly enough.

I can extend redis-store and implement this anyway, because our services need to account for this, but I was hoping to integrate this for those who would like to continue to use redis-store with what seems to be the communities recommendation for connecting to redis cluster with ruby.

In case you're interested, this is my branch with the patch: https://github.com/redis-store/redis-activesupport/compare/master...film42:gt/ignore_some_command_errors?expand=1

Copied from the branch:

+        rescue Redis::CommandError => error
+          raise unless ignored_command_errors.include?(error)
+          raise if raise_errors?
+          false
         rescue Errno::ECONNREFUSED, Errno::EHOSTUNREACH, Redis::CannotConnectError
           raise if raise_errors?
           false

If you're still thinking 👎 , that's fine too. I will close this issue and extend the gem.

[film42]

I went ahead and released https://github.com/film42/redis-cluster-activesupport as redis-cluster-activesupport.

If someone is googling for supporting redis-activesupport with redis cluster or a redis cluster proxy, you can swap out config.cache_store = :redis_store for config.cache_store = :redis_cluster_store. This is essentially the branch I linked to above but I went ahead and dropped support for things like delete_matched and fetch_multi which would not work under redis cluster.

[tubbo]

@film42 that sounds good, and for what it's worth I may merge this eventually into the main project (will definitely let you know if I do), but for now I'd like to see how the Redis Cluster thing pans out and how people end up using it. hopefully either Redis.rb or someone else will introduce support in pure Ruby, and then it becomes a matter of figuring out which Redis client you'd want to use with Redis::Store (imho it shouldn't be inheriting from Redis, rather it should be proxying method calls to the underlying Redis client, whatever that might be...)

[film42]

@tubbo I hope for the same! 🤞