finagle-redis: Use StrictKeyCommand for XDEL

Problem
Redis XDEL command is incorrectly categorized as a `StrictKeysCommand`.
The command semantics are `XDEL key ID [ID ...]`.  Using `StrictKeysCommand`,
the list of `ids` is used as `keys`.  However, XDEL only takes one key and
it should be the pkey, rather than `ids`, which are lkeys.

Solution
Use `StrictKeyCommand` for XDEL

JIRA Issues: CSL-8806

Differential Revision: https://phabricator.twitter.biz/D517291

Author: Jenny Lin

CHANGELOG.rst

@@ -13,6 +13,8 @@ Breaking API Changes
 * finagle-core: Correct the spelling of `Tracing.recordClientSendFrargmet()` to
   `Tracing.recordClientSendFragment()` ``PHAB_ID=D505617``
 
+* finagle-redis: Use `StrictKeyCommand` for XDEL ``PHAB_ID=D517291``
+
 * finagle-zipkin-scribe: The scribe.thrift file was moved to finagle-thrift/src/main/thrift under a new
   namespace. `com.twitter.finagle.thrift.scribe.(thrift|thriftscala)` ``PHAB_ID=D511471``
 

finagle-redis/src/it/README.md

@@ -13,7 +13,7 @@ Instructions
    done via https://redis.io/download or via `brew install redis`.
 
 2. That's all the setup! To run with pants, run
-   `./pants finagle/finagle-redis/src/it/scala`. To run with sbt, run
+   `./pants test finagle/finagle-redis/src/it/scala`. To run with sbt, run
    `./sbt 'project finagle-redis' 'it:testOnly'`.
 
 Note that running the tests will create the file `dump.rdb` in the directory

finagle-redis/src/main/scala/com/twitter/finagle/redis/RedisPartitioningService.scala

@@ -108,15 +108,6 @@ private[finagle] class RedisPartitioningService(
 
       case kc: KeyCommand => Seq(kc.key)
 
-      // NOTE: XDel seems to be categorized incorrectly in the hierarchy of Command traits.
-      // According to the documentation the semantics are `XDEL mystream id1 id2...`.
-      // The XDel case class has `keys` overridden as the ids, however these are lkeys,
-      // not pkeys, so it seems partitioning based on anything but the `key` field would
-      // be erroneous, therefore we can't rely on the KeysCommand case here to do things
-      // correctly, unless we want to change the implementation of the XDel command, and
-      // I'm unsure of the implications of doing that
-      case xdel: XDel => Seq(xdel.key)
-
       case kcs: KeysCommand => kcs.keys
       case _ => unsupportedCommand(command)
     }
@@ -211,7 +202,7 @@ private[finagle] class RedisPartitioningService(
 
   protected def isSinglePartition(command: Command): Future[Boolean] =
     command match {
-      case _: KeyCommand | _: XDel | _: Ping.type => Future.True
+      case _: KeyCommand | _: Ping.type => Future.True
       case _ if allKeysForSinglePartition(command) => Future.True
       case _ => Future.False
     }

finagle-redis/src/main/scala/com/twitter/finagle/redis/protocol/commands/Streams.scala

@@ -61,10 +61,10 @@ case class XTrim(key: Buf, size: Long, exact: Boolean) extends StrictKeyCommand
   }
 }
 
-case class XDel(key: Buf, ids: Seq[Buf]) extends StrictKeysCommand {
-  override def name: Buf = Command.XDEL
+case class XDel(key: Buf, ids: Seq[Buf]) extends StrictKeyCommand {
+  RequireClientProtocol(ids.nonEmpty, "ids must not be empty")
 
-  override def keys: Seq[Buf] = ids
+  override def name: Buf = Command.XDEL
 
   override def body: Seq[Buf] = Seq(key) ++ ids
 }