With redisClusterAsyncCommandArgvToNode api, client not discovering new nodes on redis master node disconnect

[rahulKrishnaM]

Hi,

I was debugging an intermittent issue with hiredis client when one of the master redis server pods was going down. The client was not able to connect back to the newer redis server pod/IP that came up after master pod gets respawned, and it was never able to recover from this state.

Digging more into the issue I was able to figure out that the particular issue was happening only when applications trigger a traffic pattern that internally invokes redisClusterAsyncCommandArgvToNode() api calls alone. Whereas, when the traffic uses redisClusterAsyncCommandArgv() api alone, there is 100% recovery for this scenario.

I looked further into the issue and was able to narrow it down to what was causing the difference in client behaviour.

It seems like the callback passed to hiredis library is redisClusterAsyncCallback() in case of redisClusterAsyncCommandArgvToNode() api and is redisClusterAsyncRetryCallback callback in case of redisClusterAsyncCommandArgv() which is making the difference.

    status = redisAsyncFormattedCommand(ac, redisClusterAsyncCallback, cad, cmd,
                                        len);

    status = redisAsyncFormattedCommand(ac, redisClusterAsyncRetryCallback, cad,
                                        cmd, len);

redisClusterAsyncRetryCallback(), if it sees a NULL reply object triggers a cluster slots command, whereas redisClusterAsyncCallback doesn't seem to be doing anything than just saving the error.

Is there a reason why we have redisClusterAsyncCallback instead of redisClusterAsyncRetryCallback for redisClusterAsyncCommandArgvToNode() api ? Can we invoke the same redisClusterAsyncRetryCallback for node specific api so that it does cluster rediscovery ?

cc: @bjosv

[bjosv]

Ah.. The ..ToNode() were introduced as a low level API primarily to be able to send commands that are not slot bound, and this was the reason for not performing retires and redirect as stated in the README. Are you sending commands without keys in your case?

I have seen usage of cluster_update_route() to manually handle cluster rediscovery in this case, but maybe it's not perfect..not after the recent changes which introduces async slotmap updates.

Would you need to have the command resent, or would it be enough if hiredis-cluster internally triggers a rediscovery so that the slotmap gets updated for future commands? I guess resending commands might be a problem since we are not really sure if the redis server has handled the command or not. It depends on error and what command is sent, e.g. we don't want to resend a INCR command for example.

[rahulKrishnaM]

Hi @bjosv Thanks for the detail.

We have a use case where we are relying on the ..ToNode() api to send the same command to all the master nodes that are present in the cluster combine the responses from individual nodes and sent back the response to application.

So, we invoke an initNodeIterator() to get the iterator begin, followed by redisClusterAsyncCommandArgvToNode() for the iterator object, till iterator becomes invalid in a while loop.

The issue we are facing is that, as soon a redis master server node is deleted, it still attempts to send the command to the stale IP and gives back timeout response. So, we are never able to respond back with success to the application.

Are you sending commands without keys in your case?

The command has keys in it, and the command structure is similar to how we pass data to redisClusterAsyncCommandArgv() api.

Would you need to have the command resent, or would it be enough if hiredis-cluster internally triggers a rediscovery so that the slotmap gets updated for future commands?

Since the master node has gone down, I guess it's fine to see 1 or 2 timeout responses since the hiredis nodes table takes a while to get updated. But we definitely want it to perform a rediscovery so that it recovers and starts sending the command to a valid node present in the cluster. Making it as closer to redisClusterAsyncCommandArgv() behaviour would be ideal I would say since end user need not worry of handling the command/response differently in both cases.

[bjosv]

But we definitely want it to perform a rediscovery so that it recovers and starts sending the command to a valid node present in the cluster.

Sounds reasonable. I'll look into it..unless you are preparing a PR already?

[rahulKrishnaM]

Thanks, please go ahead @bjosv :)