Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MOVED ERROR in pipeline-API when increasing shards in existing redis cluster #172

Open
abhinavbansal19961996 opened this issue Jun 22, 2023 · 10 comments

Comments

@abhinavbansal19961996
Copy link

abhinavbansal19961996 commented Jun 22, 2023

We call below function
redisClusterAppendCommand which calls lua script and sets and gets stuff from redis.
and then call redisClusterGetReply(cntxt, &reply).

Our code continously executes both functions... Now during above process, we increased number of shards in our existing redis cluster. Then we observed during scaling, we saw below error on printing reply:
reply->type and reply->str came out to be REDIS_REPLY_ERROR and MOVED IP Address:port respectively.

Since clients dont need to care about internal nodes/shards, should the hirendis-vip handle it like retrying to moved node? Need help on this.

NOTE: We are using hirendis-vip

If above issue is fixed in hiredis-cluster, let me know.

@zuiderkwast
Copy link
Collaborator

Hi!

Since clients dont need to care about internal nodes/shards, should the hirendis-vip handle it like retrying to moved node? Need help on this.

Yes, hiredis-cluster handles the MOVED redirects, updates the slot-to-node mapping and retries the command automatically. The user doesn't need to care about this and should not see the MOVED redirect.

NOTE: We are using hirendis-vip

If above issue is fixed in hiredis-cluster, let me know.

I don't know all the problems of hiredis-vip but we have improved many things. It's better that you try the latest version of hiredis-cluster and check if the problem is still there. If there is still a problem in hiredis-cluster, we can try to fix it.

@abhinavbansal19961996
Copy link
Author

abhinavbansal19961996 commented Jun 27, 2023

We checked and it is problem with the latest client (hiredis-cluster) also. Exactly Same MOVED error.

@zuiderkwast
Copy link
Collaborator

Interesting.

How do you call the EVAL command with arguments? Hiredis-cluster is using the first key in the EVAL command to send the command to the right node.

What is the contents of the Lua script?

@abhinavbansal19961996
Copy link
Author

abhinavbansal19961996 commented Jun 27, 2023

"EVAL %s 1 %s %b %d %d %s %d" (first %s is uuid)
Above is what we are evaulating
Below is lua script

"local status = redis.call('SET', KEYS[1], ARGV[1] .. '&%#' .. ARGV[2] .. '&%#' .. ARGV[3] .. '&%#' .. ARGV[4], 'lak', ARGV[5], 'pak'); "
"if status == false "
"then "
" local value = redis.call('GET', KEYS[1]); "
" local wowoow, kokok, lolla, aasasas = string.match(value, '(.)&%%#(.)&%%#(.)&%%#(.)') "
" if (kokok == ARGV[2]) "
" then"
" return 'DUPLICATE' "
" else "
" local poqi = {}; "
" if (kokok == '1') "
" then "
" poqi[1] = ARGV[1] "
" poqi[2] = ARGV[3] "
" poqi[3] = ARGV[4] "
" else "
" poqi[1] = wowoow "
" poqi[2] = lolla "
" poqi[3] = aasasas "
" end "
" redis.call('DEL', KEYS[1]); "
" return poqi "
" end "
"else "
" return status "
"end";

                       Note: We get this MOVED error on doing redisClusterGetReply in reply

@zuiderkwast
Copy link
Collaborator

zuiderkwast commented Jun 27, 2023

Thanks. I think there is no problem in the Lua script. I can see you call EVAL with one key and KEYS[1] is used for the key in the script. That's good.

I found out now that redisClusterAppendCommand + redisClusterGetReply does not follow MOVED-redirects. I'm sorry I didn't find out before.

The workaround is to use redisClusterCommand. It follows MOVED-redirects automatically. Another possibility is to use the async API. Or call cluster_update_route() when you get MOVED error and then try again using redisClusterAppendCommand + redisClusterGetReply.

We can to look at how we can implement redirect handling for redisClusterGetReply in the future, but there is no plan right now.

@abhinavbansal19961996
Copy link
Author

ok ok got it... Hoping to see it some day in future.

Just two questions:

  1. We are calling redisClusterAppendCommand for 100 messages, 100 times and then calling redisClusterGetReply and listening for 100 messages. So let us suppose we shift to redisClusterCommand. We will be calling redisClusterCommand for each of the 100 messages one by one. So question is, that might increase total run time right? if no, then other drawback of redisClusterCommand?

  2. cluster_update_route is not exposed right? how to use it?

@zuiderkwast
Copy link
Collaborator

  1. Yes, redisClusterCommand is slower because it waits for each reply before you can send the next command. Pipelining is faster.

    If you do 100 redisAppendCommand and 100 redisClusterGetReply, you can keep track of which commands failed and retry only those commands after update route. It is more complicated but it will be faster.

  2. cluster_update_route is internal but it is exposed in the .h file. You can call it with the cluster context cc as cluster_update_route(cc) and the return value is REDIS_OK or REDIS_ERR.

@abhinavbansal19961996
Copy link
Author

We tried doing cluster_update_route(cc) but seems like it didn't work. Errors were still there. What exactly it does cluster_update_route?

@zuiderkwast
Copy link
Collaborator

It sends CLUSTER NODES (or CLUSTER SLOTS if configured to use it instead) to a Redis node to learn about which node owns each slot. It updates an internal table in the cluster context.

@zuiderkwast zuiderkwast changed the title MOVED ERROR when increasing shards in existing redis cluster MOVED ERROR in pipeline-API when increasing shards in existing redis cluster Aug 21, 2023
@zuiderkwast
Copy link
Collaborator

Implementing redirects for the pipeline API seems like it's not strait-forward. Currently it is not planned.

Have you considered using the async API? It is very efficient and it handles redirects and updates slot mapping in the background.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants