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

Multiplexed Commands Del, MSet, SInter and SUnion #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

loloreyn
Copy link

Implementing multiplexed commands (Del, MSet, SInter and SUnion as well as SAdd, which is needed to test SInter and SUnion).

Tests with server done.

…ll as SAdd, which is needed to test SInter and SUnion).

Tests with server done.
this.keys = keys;
}

// not really necessary, it just takes the pain away from the user of having to create an ArrayList just for 1 key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this comment..?

@andlaz
Copy link
Contributor

andlaz commented Aug 22, 2014

I noticed in tests you are only using assertTrue();
please make liberal use of assertEquals for asserting equivalence, fail() for failing a test on some complex condition ( say in an if-block; but use assertEquals() where possible )

@loloreyn
Copy link
Author

about the assertTrue() in Redis2TextProtocol.java, I can't do
assertEquals(execCommandBytes.toByteArray(), encoded);
since it compares the reference of the objects too. but if I do
assertTrue(Arrays.equals(encoded, execCommandBytes.toByteArray()));
the tests pass since it compares the content.

@loloreyn loloreyn closed this Aug 22, 2014
@loloreyn loloreyn reopened this Aug 22, 2014
if(multiBulkReply.value().size()==0) return new MultiBulkReply(new ArrayList<Reply>());

base.retainAll(multiBulkReply.value());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this loop.

  • If you come upon a MultiBulkReply thats empty, you discard everything and return a new empty "combined" reply
  • otherwise you remove all Replies from base that are not in the Reply you are iterating on

I think the original intention here was to create a single MultiBulkReply for the MultiBulkReply objects of the child SInters - how is this fulfilling that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sinter command returns a set that is an intersection of all the sets asked through the keys. If even one of them is an empty set, the returned set will also be empty. So there is no point in continuing in the loop for x times. You just return an MultiBulkReply that has an empty set as value.

from the redis doc:
Keys that do not exist are considered to be empty sets. With one of the keys being an empty set, the resulting set is also empty (since set intersection with an empty set always results in an empty set).

@andlaz
Copy link
Contributor

andlaz commented Aug 25, 2014

Please remove the Multiplexing... test methods from AbstractClientTest. We'll move the existing methods to a benchmark and re-do this class with mocking so it can properly test Client implementations without a Redis connection.

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

Successfully merging this pull request may close these issues.

3 participants