zinterstore / zunionstore work on cluster when they shouldn't #1581

Closed
mgravell opened this Issue Mar 6, 2014 · 3 comments

Comments

Projects
None yet
3 participants

mgravell commented Mar 6, 2014

Same story as #1580

zadd b 1 "one"
zadd b 2 "two"
zadd b 3 "three"

then:

zinterstore c 1 b

or

zunionstore c 1 b

I would expect "ERR Multi keys request invalid in cluster", which would be consistent with "sdiffstore", "sunionstore" and "sinterstore" (which work correctly, IMO)

antirez added this to the Redis 3.0 milestone Mar 6, 2014

Contributor

mattsta commented Mar 7, 2014

Thanks for noticing the error here!

I found a fix and will get it out soon:

matt@ununoctium:~/repos/redis/src% ./redis-cli -p 25406
127.0.0.1:25406> zadd {hi}b 1 one
(integer) 1
127.0.0.1:25406> zadd {hi}b 2 two
(integer) 1
127.0.0.1:25406> zadd {hi}b 3 three
(integer) 1
127.0.0.1:25406> zinterstore {hi}store 1 {hi}b
(integer) 3
127.0.0.1:25406> zinterstore XXstore 1 {hi}b
(error) CROSSSLOT Keys in request don't hash to the same slot

(Note the use of {hi} allowing us to pin keys to individual hash slots making multi-key operations valid in the cluster (as long as all keys in the same command hash to the same slot)).

@mattsta mattsta added a commit to mattsta/redis that referenced this issue Mar 7, 2014

@mattsta mattsta Fix key extraction for z{union,inter}store
The previous implementation wasn't taking into account
the storage key in position 1 being a requirement (it
was only counting the source keys in positions 3 to N).

Fixes antirez/redis#1581
f0782a6

antirez closed this in #1586 Mar 10, 2014

Owner

antirez commented Mar 10, 2014

p.s. note, this PR breaks EVAL / EVALSHA. Fixing!

Owner

antirez commented Mar 10, 2014

Should be fine now, btw we have a new command, DEBUG CMDKEYS ... that can be used to debug and test this stuff.

@antirez antirez added a commit that referenced this issue Mar 11, 2014

@mattsta @antirez mattsta + antirez Fix key extraction for z{union,inter}store
The previous implementation wasn't taking into account
the storage key in position 1 being a requirement (it
was only counting the source keys in positions 3 to N).

Fixes antirez/redis#1581
14f77b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment