MSETNX behaves wrong about curiodb HOT 5 CLOSED

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?

from curiodb.

Ok I found the bug - it was as simple as the wrong method name being overridden in AggregateMSetNX - it should have been broadcastArgs instead of keys. Basically if you study AggregateBroadcast (which AggregateMSetNX eventually extends), you'll see it takes over the keys method with an integer range (one per KeyNode, since it's communicating with all of them), and then uses its own broadcastArgs to implement command args (or keys).

I also updated the comments to try and explain this more clearly. Thanks again for the report! Looking forward to the next bug. :-)

from curiodb.

Fixed in 3a53d78

from curiodb.

What about tests for a java or scala redis client?

from curiodb.

Great idea
On 10/07/2015 12:32 PM, "tramchamploo" [email protected] wrote:

What about tests for a java or scala redis client?

—
Reply to this email directly or view it on GitHub
#6 (comment).

from curiodb.