FailoverClusterClient does not retry during failover related NOREPLICAS errors

[Josh-McWilliam]

3-redis 3-sentinel cluster with min-replicas-to-write of 1
When continuously writing data with a FailoverClusterClient, executing a failover via the sentinels will cause the client error out with a NOREPLICAS error regardless of the retry settings. This typically resolves itself within ~50ms as the replicas are reassigned to the new master.

Expected Behavior

During a failover the clusterclient just retry with backoff on NOREPLICAS errors (within the bounds or the maxretries/maxredirect setting), this will give it time to reassign remaining nodes as replicas and resolve the issue.

Current Behavior

NOREPLICAS errors cause .Set to return an err and does not attempt to retry regardless of settings. A manual retry loop solves this issue though it shouldn't be needed. As far as I can tell this issue also occurs with the regular FailoverClient, not just the FailoverClusterClient

Possible Solution

There exists has a preset list of errors in shouldRetry (error.go#79), modify this function (or the calling function if you want to differentiate the write/read behavior) to allow retrying on NOREPLICAS

Steps to Reproduce

(This was originally encountered on a k8s cluster, though I can try create a demo if the provided steps are not sufficient)

1. Redis cluster with 3 nodes, 3 sentinels, 1 primary and a min-replicas-to-write of 1
2. Simple for loop with a FailoverClusterClient pointed at one of the sentinels constantly sending set requests
3. have the sentinel failover redis-cli -p 26379 sentinel failover redismaster
4. Client will eventually fail with a NOREPLICAS error.

Context (Environment)

This was originally encountered on a local kind cluster, it is possible the issue may be less common in the case of faster hardware/networking that allows sentinels/replicas to reassign slaves faster
While this does seem to happen with a standard FailoverClient, I have not done much testing with different config options for the FailoverClient

Detailed Description

The Failover/FailoverCluster client should not fail during a failover, a specific catch for this error or a global/fallback retry setting would be appreciated.

[ndyakov]

@Josh-McWilliam Thank you for reporting this. My initial response here would be that this is an error from redis that should be delivered to the user, then if the user would like to retry in their application code, sure - they can, but the client itself should be a thin layer of abstraction and this error is not one we should retry on. There is no way for the client to know what is the server configuration and IF this NOREPLICAS error can be something the server will recover from or not.

cc @ofekshenawa maybe you can take a look next week and share your input as well

[Josh-McWilliam]

(Quick note, this does also occur with just 1-2 sentinels, not specifically 3)
Thanks for the quick response!
I'm not 100% sure I follow the argument there though, this is (as far as I understand) an issue that will occur whenever an attempt is made to write shortly after/during a failover on any cluster that has a minimum replicas >= 1 (Although cluster latency likely has an impact on the chance of encountering this).
Even if this error is not one the server can recover from in its current configuration a retry still makes sense within the bounds of the max retries limit (at least, to the same degree that it is reasonable to retry on READONLY and MASTERDOWN as is currently already done). If this is a failover client it should reasonably be able to recover from failover related errors.

This isn't really a hill I will die on as a manual retry loop is an easy workaround, but I do think it seems strange to retry on something like READONLY but not this.