Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

finagle-redis: Add SINTER command to redis client #248

Closed
wants to merge 4 commits into from

5 participants

@jbripley

Hi,

I noticed that the SINTER command was missing from the finagle-redis client when I tried to use it, so I added it in.

Tested with Scala 2.10.0 and Redis server version 2.6.12.

...ain/scala/com/twitter/finagle/redis/SetCommands.scala
@@ -96,4 +96,21 @@ trait Sets { self: BaseClient =>
case MBulkReply(messages) => Future.value(ReplyFormat.toChannelBuffers(messages))
case EmptyMBulkReply() => Future.Nil
}
+
+ /**
+ * Returns the members of the set resulting from the intersection of all the given sets.
+ *
+ * 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).
+ *
+ * Throws an exception if no or empty keys are provided.
+ *
+ * @param keys list of keys to intersect
+ * @return List with members of the resulting set
+ */
+ def sInter(keys: Seq[ChannelBuffer]): Future[Seq[ChannelBuffer]] =
@mosesn Collaborator
mosesn added a note

To keep the api consistent, this should return an immutable Set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mosesn mosesn commented on the diff
...ain/scala/com/twitter/finagle/redis/SetCommands.scala
@@ -96,4 +96,21 @@ trait Sets { self: BaseClient =>
case MBulkReply(messages) => Future.value(ReplyFormat.toChannelBuffers(messages))
case EmptyMBulkReply() => Future.Nil
}
+
+ /**
@mosesn Collaborator
mosesn added a note

please make sure your lines are < 100 characters long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ain/scala/com/twitter/finagle/redis/SetCommands.scala
@@ -96,4 +96,21 @@ trait Sets { self: BaseClient =>
case MBulkReply(messages) => Future.value(ReplyFormat.toChannelBuffers(messages))
case EmptyMBulkReply() => Future.Nil
}
+
+ /**
+ * Returns the members of the set resulting from the intersection of all the given sets.
+ *
+ * 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).
+ *
+ * Throws an exception if no or empty keys are provided.
@mosesn Collaborator
mosesn added a note

This comment seems to suggest that it will throw an exception if we try to query a key where there isn't a Set, but the tests contradict that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mosesn
Collaborator

Could you add some NaggatiSpec tests? Other than that, and a few other small changes, looks good!

Joakim Bodin finagle-redis: Updated SINTER command implementation after pull reque…
…st feedback

- Now returns an immutable Set, same as SMEMBERS

- Wrapped long lines

- Updated SINTER command documentation to be more clear when exceptions are thrown

- Added NaggatiSpec tests and updated ClientSpec tests for changed return value

- Add SINTER to commandMap
5a3eb74
...ain/scala/com/twitter/finagle/redis/SetCommands.scala
@@ -96,4 +96,25 @@ trait Sets { self: BaseClient =>
case MBulkReply(messages) => Future.value(ReplyFormat.toChannelBuffers(messages))
case EmptyMBulkReply() => Future.Nil
}
+
+ /**
+ * Returns the members of the set resulting from the intersection of all
+ * the given sets.
+ *
+ * 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).
+ *
+ * Throws an exception if the `keys` Seq is empty or if all of the values
+ * included in the `keys` Seq are empty.
@mosesn Collaborator
mosesn added a note

It won't throw an exception if the values included in the keys Seq are empty, I think.

@jbripley
jbripley added a note

From reading the code in KeysCommand, it looked like empty key values would throw an exception as well. My unit test seems to indicate that this is true as well.

I can remove that part of the comment though, if you think it'll only confuse people.

@jbripley
jbripley added a note

Just noticed that the comment should probably read "any" instead of "all" in the "if all of the values included in the keys Seq are empty" part of the comment.

@mosesn Collaborator
mosesn added a note

Ah, when you said the values of the key, I thought you meant if the Sets that the keys are pointing to are empty, not the key string. It's a bug in finagle-redis that they throw though, is that something that we can stick in this commit to fix, or do you think it would end up being a bigger task? I think it might be clearer to say, "If any of the keys passed as params are empty, this will throw."

@jbripley
jbripley added a note

I don't feel confident enough about the finagle-redis codebase to get started on that and besides, I would probably want to do that in a separate pull request.

I'll update the comment as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ain/scala/com/twitter/finagle/redis/SetCommands.scala
((7 lines not shown))
+ * the given sets.
+ *
+ * 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).
+ *
+ * Throws an exception if the `keys` Seq is empty or if all of the values
+ * included in the `keys` Seq are empty.
+ *
+ * @param keys list of keys to intersect
+ * @return Set of members from the resulting intersection
+ */
+ def sInter(keys: Seq[ChannelBuffer]): Future[ImmutableSet[ChannelBuffer]] =
+ doRequest(SInter(keys)) {
+ case MBulkReply(messages) =>
+ Future.value(ReplyFormat.toChannelBuffers(messages) toSet)
@mosesn Collaborator
mosesn added a note

style nit: avoid postfix operators. ReplyFormat.toChannelBuffers(messages).toSet is more legible.

@jbripley
jbripley added a note

Yep, I don't really like that syntax either and will fix it. I was just following the rest of the code in the file though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asrinivas

lgtm!

@roanta
Collaborator

merged internally, should sync up here next release.

@jbripley jbripley closed this
@jbripley jbripley reopened this
@evnm evnm closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 5, 2014
  1. finagle-redis: Add SINTER command to redis client

    Joakim Bodin authored
  2. finagle-redis: Updated SINTER command implementation after pull reque…

    Joakim Bodin authored
    …st feedback
    
    - Now returns an immutable Set, same as SMEMBERS
    
    - Wrapped long lines
    
    - Updated SINTER command documentation to be more clear when exceptions are thrown
    
    - Added NaggatiSpec tests and updated ClientSpec tests for changed return value
    
    - Add SINTER to commandMap
Commits on Mar 6, 2014
  1. finagle-redis: Syntax cleanup of toSet call

    Joakim Bodin authored
This page is out of date. Refresh to see the latest.
View
21 finagle-redis/src/main/scala/com/twitter/finagle/redis/SetCommands.scala 100644 → 100755
@@ -96,4 +96,25 @@ trait Sets { self: BaseClient =>
case MBulkReply(messages) => Future.value(ReplyFormat.toChannelBuffers(messages))
case EmptyMBulkReply() => Future.Nil
}
+
+ /**
@mosesn Collaborator
mosesn added a note

please make sure your lines are < 100 characters long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * Returns the members of the set resulting from the intersection of all
+ * the given sets.
+ *
+ * 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).
+ *
+ * Throws an exception if the `keys` Seq is empty or if any of the keys
+ * passed as params are empty.
+ *
+ * @param keys list of keys to intersect
+ * @return set of members from the resulting intersection
+ */
+ def sInter(keys: Seq[ChannelBuffer]): Future[ImmutableSet[ChannelBuffer]] =
+ doRequest(SInter(keys)) {
+ case MBulkReply(messages) =>
+ Future.value(ReplyFormat.toChannelBuffers(messages).toSet)
+ case EmptyMBulkReply() => Future.value(ImmutableSet())
+ }
}
View
3  finagle-redis/src/main/scala/com/twitter/finagle/redis/protocol/Command.scala 100644 → 100755
@@ -124,6 +124,7 @@ object Commands {
val SREM = "SREM"
val SPOP = "SPOP"
val SRANDMEMBER = "SRANDMEMBER"
+ val SINTER = "SINTER"
// Transactions
val DISCARD = "DISCARD"
@@ -241,6 +242,7 @@ object Commands {
SREM -> {SRem(_)},
SPOP -> {SPop(_)},
SRANDMEMBER -> {SRandMember(_)},
+ SINTER -> {SInter(_)},
// transactions
DISCARD -> {_ => Discard},
@@ -375,6 +377,7 @@ object CommandBytes {
val SREM = StringToChannelBuffer("SREM")
val SPOP = StringToChannelBuffer("SPOP")
val SRANDMEMBER = StringToChannelBuffer("SRANDMEMBER")
+ val SINTER = StringToChannelBuffer("SINTER")
// Transactions
val DISCARD = StringToChannelBuffer("DISCARD")
View
10 finagle-redis/src/main/scala/com/twitter/finagle/redis/protocol/commands/Sets.scala 100644 → 100755
@@ -96,3 +96,13 @@ object SRandMember {
def apply(args: Seq[Array[Byte]]): SRandMember =
SRandMember(GetMonadArg(args, CommandBytes.SRANDMEMBER))
}
+
+case class SInter(keys: Seq[ChannelBuffer]) extends StrictKeysCommand {
+ val command = Commands.SINTER
+ override def toChannelBuffer = RedisCodec.toUnifiedFormat(CommandBytes.SINTER +: keys)
+}
+
+object SInter {
+ def apply(args: => Seq[Array[Byte]]) =
+ new SInter(args.map(ChannelBuffers.wrappedBuffer))
+}
View
14 finagle-redis/src/test/scala/com/twitter/finagle/redis/NaggatiSpec.scala 100644 → 100755
@@ -618,6 +618,20 @@ class NaggatiSpec extends SpecificationWithJUnit {
}
}
}
+ "SINTER" >> {
+ codec(wrap("SINTER\r\n")) must throwA[ClientError]
+ unwrap(codec(wrap("SINTER key\r\n"))) {
+ case SInter(keys) => {
+ CBToString(keys(0)) mustEqual "key"
+ }
+ }
+ unwrap(codec(wrap("SINTER key1 key2\r\n"))) {
+ case SInter(keys) => {
+ CBToString(keys(0)) mustEqual "key1"
+ CBToString(keys(1)) mustEqual "key2"
+ }
+ }
+ }
}
} // inline
View
33 finagle-redis/src/test/scala/com/twitter/finagle/redis/integration/ClientSpec.scala 100644 → 100755
@@ -224,6 +224,39 @@ class ClientSpec extends SpecificationWithJUnit {
Await.result(client.get(baz)) mustEqual Some(StringToChannelBuffer("fbaz"))
}
+ "set intersection variations" in {
+ val a = StringToChannelBuffer("a")
+ val b = StringToChannelBuffer("b")
+ val c = StringToChannelBuffer("c")
+ val d = StringToChannelBuffer("d")
+ val e = StringToChannelBuffer("e")
+
+ val fooMembers = CollectionSet(a, b, c, d)
+ Await.result(client.sAdd(foo, fooMembers.toList))
+ Await.result(client.sAdd(boo, List(c)))
+ Await.result(client.sAdd(baz, List(a, c, e)))
+ Await.result(client.sAdd(moo, List(a, b)))
+
+ // Should intersect a single value
+ Await.result(client.sInter(Seq(foo, boo, baz))) mustEqual CollectionSet(c)
+
+ // Has no intersection
+ Await.result(client.sInter(Seq(boo, moo))) mustEqual CollectionSet.empty
+
+ // bar is not a known key
+ Await.result(client.sInter(Seq(foo, bar))) mustEqual CollectionSet.empty
+
+ // neither num or bar is a known key
+ Await.result(client.sInter(Seq(num, bar))) mustEqual CollectionSet.empty
+
+ // Only one key will give itself as intersection
+ Await.result(client.sInter(Seq(foo))) must containAll(fooMembers)
+
+ // At least one non-empty key is required
+ Await.result(client.sInter(Seq())) must throwA[ClientError]
+ Await.result(client.sInter(Seq(StringToChannelBuffer("")))) must throwA[ClientError]
+ }
+
"new set syntax variations" in {
Await.result(client.setExNx(foo, 10L, bar)) mustEqual true
Await.result(client.get(foo)) mustEqual Some(bar)
Something went wrong with that request. Please try again.