Possible to add a common error superclass for redis-cluster-client errors? Or use with_config consistently?

[gstokkink]

Hi team,

I've noticed that all errors in https://github.com/redis-rb/redis-cluster-client/blob/master/lib/redis_client/cluster/errors.rb directly inherit from ::RedisClient::Error. For some of our error handling, it's useful to be able to distinguish where errors are coming from (for instance, what Redis connection, and whether it's a cluster or not). Normally I'd look at the config that is associated with the redis client error (through https://github.com/redis-rb/redis-client/blob/master/lib/redis_client.rb#L105), but it seems like the choice was made not to use with_config to raise errors in this gem. If that is done for good reasons, perhaps you may consider adding a RedisClient::Cluster::Error superclass that inherits from ::RedisClient::Error, and let all errors in this gem inherit from that instead?

[supercaracal]

Hello,

It seems that the with_config method feature isn't suitable for our cluster errors. As you said, I'd say that it would be better to add RedisClient::Cluster::Error as a subclass from RedisClient::Error and our cluster errors should inherit it.

Although there is a middleware which may be related this issue, I think the above inheritance way is simpler.
https://github.com/redis-rb/redis-cluster-client/blob/master/lib/redis_client/cluster/error_identification.rb

[gstokkink]

@supercaracal currently I use the id field of the Redis config to identify what Redis configuration raised a particular error. I can then do things like rescue RedisClient::Error => e; raise e unless e.config.id == SOME_VALUE; end to selectively ignore certain Redis errors and make things more fault tolerant. Unfortunately this does not work for errors from this gem because, like I said, the config isn't set on the errors. I only have a single configuration that connects to a Redis store in cluster mode, so the inheritance way would also fix it, though it's less elegant.

By the way, just wondering why the cluster errors in this gem cannot use the with_config approach? Is it because there can be multiple nodes involved? Isn't there always at least a cluster config (https://github.com/redis-rb/redis-cluster-client/blob/master/lib/redis_client/cluster_config.rb) present? This cluster config would include the id field. I'd prefer not using the middleware yeah, it's probably quite some performance overhead?

[supercaracal]

Is it because there can be multiple nodes involved?

Yes, it is. Although the with_config method was added by the following pull request, it looks like it is used by only connection errors so far.

• Improve error log message output redis-client#178

Peronally, I don't want to add a dependency between a config and errors just for the error identification. I'd say that error handlings should be done by error types. But I'm not pretty sure of your use case, do you treat multiple clusters or mixed type redis instances? If so, I think there can only be a way to use the with_config method on every our errors.

it's probably quite some performance overhead?

I thought so. So I made the middleware optional since the following pull request.

• fix: make the error identification middleware optional because of the use case #344

[gstokkink]

@supercaracal yeah it would be nice if with_config was used wherever possible. Makes it easy to see which Redis connections are failing, and allows possible recovery. We currently have multiple Redis stores running, some in cluster mode, some not, for various purposes. For some of these stores it's okay if they're not available for whatever reason (perhaps a system upgrade), for others not so much. That's why it's useful for us to see exactly which Redis stores are having issues. Looking at the exception class is not enough since that does not allow us to pinpoint where the exception is originating from, and most code deals with multiple stores at once.

[supercaracal]

I understood. Thank you for sharing your use case. I'll work on it ASAP.

[gstokkink]

@supercaracal thank you very much for fixing this! It works beautifully 😄