Nordix / hiredis-cluster

C client library for Valkey/Redis Cluster. This project is used and sponsored by Ericsson. It is a fork of the now unmaintained hiredis-vip.
BSD 3-Clause "New" or "Revised" License
87 stars 42 forks source link

When is the support for redis 7 coming? Any recommendation for which redis client to use for redis version 7? #155

Closed abhinavbansal19961996 closed 1 year ago

abhinavbansal19961996 commented 1 year ago

When is the support for redis 7 coming? Any recommendation for which redis client to use for redis version 7?

zuiderkwast commented 1 year ago

Which features or commands from redis 7 do you need?

abhinavbansal19961996 commented 1 year ago

We are currently using Hiredis-vip and we want all of those commands which we are using in redis 6 to work in redis 7. I read that redis 7 is not backward compatible. So I dont need any extra features of redis 7. I want my redis client https://github.com/vipshop/hiredis-vip to work for both 7 and 6.

abhinavbansal19961996 commented 1 year ago

Just to give you list we are using currently: redisClusterContextInit redisClusterSetOptionAddNodes redisClusterSetOptionRouteUseSlots redisClusterSetOptionConnectTimeout redisClusterSetOptionTimeout redisClusterGetReply redisClusterConnect2 redisClusterAppendCommand (LUA Script)

zuiderkwast commented 1 year ago

I read that redis 7 is not backward compatible.

Where did you read this? In the Release nodes (https://github.com/redis/redis/blob/7.0/00-RELEASENOTES) there are only some very obscure cases that are listed as breaking changes. Are you affected by any of those?

I don't think any of those breaking changes for Redis 7 affect the implementation of clients, so there are no changes needed in the clients as far as I can tell, but please let me know if I'm wrong.

bjosv commented 1 year ago

We run the tests towards a range of Redis versions in CI, like 7.0.0. If there is anything you have found missing we should fix that.

zuiderkwast commented 1 year ago

To be clear, hiredis-cluster is fully backwards compatible with hiredis-vip. We only added bug fixes and more features. I think hiredis-cluster is better for all Redis versions, for Redis Cluster. If you use Redis Standalone, use the simple hiredis client.

abhinavbansal19961996 commented 1 year ago

We have a cluster of redis. When calling below function: redisClusterConnect2(r->ctxt); We are getting below error for redis 7:

Command(cluster slots) reply error: nodes sub_reply is not an correct array.

abhinavbansal19961996 commented 1 year ago

I tested by running on redis cluster version 7 and 6 locally. For 7 it was throwing above error. Any idea, when will fix be applied? We have a deadline or we might have to move to different client or understand this code better and apply patch on our end

zuiderkwast commented 1 year ago

We can apply a fix as soon as we have a fix. First we must understand the problem. This message is very helpful:

Command(cluster slots) reply error: nodes sub_reply is not an correct array.

It is coming from parse_cluster_slots in hircluster.c, which is the function that handles the reply of the CLUSTER SLOTS command. Hiredis-cluster sends this command to Redis and Redis replies with a structure that has an unexpected format. I can't understand exactly why it is happening. There is no change in Redis 7 that can explain this.

Can you connect to Redis using redis-cli and send CLUSTER SLOTS and post the output here?

Then we can understand exactly what Redis is sending. Maybe there is some configuration that affects the result. Because normally we don't get this problem with Redis 7. We have automatic tests with Redis 7.0.0, see here: https://github.com/Nordix/hiredis-cluster/actions/runs/5320977474/jobs/9635455795

abhinavbansal19961996 commented 1 year ago

I think i got the issue. Since we are using hirendis-vip, there is a bug in hirendis-vip, I checked the code. https://redis.io/commands/cluster-slots/ In above as mentioned, New fields have been added and the below condition will never be true for redis 7 since in redis 7 total length is 4 instead of 3: https://github.com/vipshop/hiredis-vip/blob/master/hircluster.c#LL856C22-L856C46 Let me know if you think my understanding is correct,

Let me try upgrading our library to hiredis-cluster since i see in hirendis-cluster, these checks are updated.

zuiderkwast commented 1 year ago

That's great! It makes sense.

I tried to find in which commit we changed this. Now I found it. It was fixed here: 0e741c6dd80063b76ee0e19792d489d3056fb0f9. The fix is included in 0.8.0 and later.

I'm closing the issue again, but feel free to open it again if it still doesn't work.