stephenmcd
commented
9 years ago The idea with using broadcast is that it optimizes for a large number of keys. If we didn't use broadcast, we would have to run EXISTS for every individual key, which would mean num messages == num keys. Instead we broadcast all keys to all KeyNodes - so num messages == num keynodes. Since each KeyNode then gets all keys, some which don't map to it, they should just return false values in their responses for each of the keys that don't map to them, which is fine - so long as we get back true values for the keys that do exist, from the KeyNodes they belong to.
Having said all that, I can see the bug, but I don't think it's due to using broadcast, which is the intention here - need to keep on digging, let's see who finds it first.
Also yes, we definitely need tests! I wonder if we can somehow leverage an existing set of tests for Redis? I tried using the TCL tests for Redis itself, but I couldn't get them to run against an external server, and they're probably too exhaustive anyway. Can you think of any other spots we mind find a good set of general Redis tests?
tramchamploo
127.0.0.1:6379> get a (nil) 127.0.0.1:6379> get b (nil) 127.0.0.1:6379> get c (nil) 127.0.0.1:6379> set b b OK 127.0.0.1:6379> msetnx a a b b c c (integer) 1 127.0.0.1:6379> get a "a" 127.0.0.1:6379> get b "b" 127.0.0.1:6379> get c "c"
The reason is AggregateMSetNX extends AggregateBroadcast which overrides begin method, it just check Command's first key. So if I set the second key already, it doesn't validate existence right. So is it really necessary to broadcast to all KeyNodes for behaviors like MSETNX? Also is there a plan to write some test cases?