clarify redirect vs retry error messages and variables about hiredis-cluster HOT 7 CLOSED

Sounds good. There is no change to the API (e.g. rename config) right?

from hiredis-cluster.

Sounds good. There is no change to the API (e.g. rename config) right?

To be consistent, I think we should change the API as well, for example redisClusterSetOptionMaxRedirect and the redisClusterContext#max_redirect_count variable name. afaics all "max redirect" should be replaced with "max retries".

from hiredis-cluster.

Right... We're still in beta so I guess we can do changes like this, but we should be very clear about the change in release notes, etc. @bjosv WDYT?

from hiredis-cluster.

Yes, I have been wanting to change this as well, but since it was a breaking change it did not progress. The current state is legacy from hiredis-vip but its a bit confusing, so I think it would be worth it.

We should change for both async and sync api (pipeline & wait for rsp), but it should be an easy search for where max_redirect_count is used.

from hiredis-cluster.

We could define a macro #define max_redirect_count max_retry_count to make old code work during a transition period, e.g. we add it in 0.7 and we remove it again in 0.8. :-)

Since any code using this won't compile if it's not updated, I think it's safe to change it. It's not going to be silently ignored or so.

from hiredis-cluster.

We should change for both async and sync api (pipeline & wait for rsp), but it should be an easy search for where max_redirect_count is used.

Yep.

Since any code using this won't compile if it's not updated, I think it's safe to change it. It's not going to be silently ignored or so.

Yep, exactly - it won't break anything silently and is easy to fix by renaming the occurrences.

@bjosv: did you start working on that, or should I prepare a PR?

from hiredis-cluster.

@bjosv: did you start working on that, or should I prepare a PR?

No, I did not start any. It would be splendid if you like to prepare a PR for it.

from hiredis-cluster.