Redis [QUESTION] Why is the SET command considered movablekeys

Hi.

I just upgraded to using redis-cluster 7.0.0 and can see that my code via redis-py is now making an extra jump to the cluster for every set call. I tracked this down to the set command having the flag movablekeys. The set command appears to always have the key in the same place, and that any client should be able to parse the key from the command based on the other parameters.

Comment From: oranagra

ohh my... looks like this is an unintended side effect of #10148

Comment From: oranagra

BTW, i think redis-py could / should have done better. AFAIK, movablekeys doesn't mean that no keys can be found using firstkey/lastkey, just that not all keys can be found. https://redis.io/commands/command/ so for the purpose of redis cluster routing, redis-py could still determine the key name of one key, and still do the routing (even if it doesn't know the key names of other keys). @chayim FYI.

Comment From: chayim

@dvora-h We keep talking about fixing the cluster implementation. This looks like the push we needed.

Comment From: enjoy-binbin

oops, i can see that, i introduced the bug... so the easiest way to fix it is we skip the SET in populateCommandMovableKeys (but it doesn't look like it will be the best fix)

command info set
1)  1) "set"
    2) (integer) -3
    3) 1) write
       2) denyoom
       3) movablekeys

Comment From: oranagra

I thought about that and I didn't like it. (referring to specific commands). Instead, we should either add some exception logic for commands with VARIABLE_FLAGS, or better yet, remove the implicit MOVABLEKEYS dependency of the get_proc and instead rely on the INCOMPLETE flag.

Comment From: enjoy-binbin

now we have these MOVABLEKEYS commands (only migrate was marked with INCOMPLETE): evalsha zunionstore fcall bitfield georadius sort zdiffstore zintercard xread sort_ro xreadgroup blmpop evalsha_ro sintercard georadiusbymember zinter zmpop fcall_ro zunion eval_ro zinterstore set migrate eval bzmpop zdiff lmpop

btw, why we are not flag these commands with MOVABLEKEYS flag in .json

Comment From: oranagra

ohh, my bad. INCOMPLETE is about incomplete key-specs. but movablekeys is about "incomplete" [first, last, step]. so we still need the implicit logic (checking if there are non-range specs), but in addition, we need to add some condition to avoid marking trivial range specs. i think we should rely on the VARIABLE_FLAGS flag.

(i'm AFK, shooting ideas from my memory, sorry)