Use same strategy for both client-side sharding and redis-cluster.

This change is possible because, after a few changes in redis-cluster,
our default cluster strategy used for client side sharding and the one
used for redis-cluster turned out to be exactly the same, except for
the hashing function used to calculate distribution.

Actually some checks used to enforce correctness are redundant in the
context of redis-cluster (e.g. the one used to make sure that keys in
requests performing cross-keys operations will hash to the same slot,
which is performed by the server) so we could also add a more dumb and
permissive cluster strategy that relies on checks performed by Redis.

The only difference between the client-side sharding strategy and the
one used for redis-cluster, aside from the hash function, is in the
rules used for extracting hash tags from keys since empty tags {} are
considered valid by redis-cluster. In v1.0.0 the strategy used for our
client-side sharding cluster will reflect this change, but we will not
change it in v0.8 since it can be considered a breaking changes as it
can potentially affect existing cluster deployments.

lib/Predis/Cluster/PredisClusterHashStrategy.php

@@ -23,8 +23,8 @@ use Predis\Command\ScriptedCommand;
  */
 class PredisClusterHashStrategy implements CommandHashStrategyInterface
 {
-    private $commands;
-    private $hashGenerator;
+    protected $commands;
+    protected $hashGenerator;
 
     /**
      * @param HashGeneratorInterface $hashGenerator Hash generator instance.

lib/Predis/Cluster/RedisClusterHashStrategy.php

@@ -12,6 +12,7 @@
 namespace Predis\Cluster;
 
 use Predis\Cluster\Hash\CRC16HashGenerator;
+use Predis\Cluster\Hash\HashGeneratorInterface;
 use Predis\Command\CommandInterface;
 use Predis\Command\ScriptedCommand;
 
@@ -21,286 +22,14 @@ use Predis\Command\ScriptedCommand;
  *
  * @author Daniele Alessandri <suppakilla@gmail.com>
  */
-class RedisClusterHashStrategy implements CommandHashStrategyInterface
+class RedisClusterHashStrategy extends PredisClusterHashStrategy
 {
-    private $commands;
-    private $hashGenerator;
-
-    /**
-     *
-     */
-    public function __construct()
-    {
-        $this->commands = $this->getDefaultCommands();
-        $this->hashGenerator = new CRC16HashGenerator();
-    }
-
-    /**
-     * Returns the default map of supported commands with their handlers.
-     *
-     * @return array
-     */
-    protected function getDefaultCommands()
-    {
-        $getKeyFromFirstArgument = array($this, 'getKeyFromFirstArgument');
-        $getKeyFromAllArguments = array($this, 'getKeyFromAllArguments');
-
-        return array(
-            /* commands operating on the key space */
-            'EXISTS'                => $getKeyFromFirstArgument,
-            'DEL'                   => $getKeyFromAllArguments,
-            'TYPE'                  => $getKeyFromFirstArgument,
-            'EXPIRE'                => $getKeyFromFirstArgument,
-            'EXPIREAT'              => $getKeyFromFirstArgument,
-            'PERSIST'               => $getKeyFromFirstArgument,
-            'PEXPIRE'               => $getKeyFromFirstArgument,
-            'PEXPIREAT'             => $getKeyFromFirstArgument,
-            'TTL'                   => $getKeyFromFirstArgument,
-            'PTTL'                  => $getKeyFromFirstArgument,
-            'SORT'                  => $getKeyFromFirstArgument, // TODO
-
-            /* commands operating on string values */
-            'APPEND'                => $getKeyFromFirstArgument,
-            'DECR'                  => $getKeyFromFirstArgument,
-            'DECRBY'                => $getKeyFromFirstArgument,
-            'GET'                   => $getKeyFromFirstArgument,
-            'GETBIT'                => $getKeyFromFirstArgument,
-            'MGET'                  => $getKeyFromAllArguments,
-            'SET'                   => $getKeyFromFirstArgument,
-            'GETRANGE'              => $getKeyFromFirstArgument,
-            'GETSET'                => $getKeyFromFirstArgument,
-            'INCR'                  => $getKeyFromFirstArgument,
-            'INCRBY'                => $getKeyFromFirstArgument,
-            'INCRBYFLOAT'           => $getKeyFromFirstArgument,
-            'SETBIT'                => $getKeyFromFirstArgument,
-            'SETEX'                 => $getKeyFromFirstArgument,
-            'MSET'                  => array($this, 'getKeyFromInterleavedArguments'),
-            'MSETNX'                => array($this, 'getKeyFromInterleavedArguments'),
-            'SETNX'                 => $getKeyFromFirstArgument,
-            'SETRANGE'              => $getKeyFromFirstArgument,
-            'STRLEN'                => $getKeyFromFirstArgument,
-            'SUBSTR'                => $getKeyFromFirstArgument,
-            'BITCOUNT'              => $getKeyFromFirstArgument,
-
-            /* commands operating on lists */
-            'LINSERT'               => $getKeyFromFirstArgument,
-            'LINDEX'                => $getKeyFromFirstArgument,
-            'LLEN'                  => $getKeyFromFirstArgument,
-            'LPOP'                  => $getKeyFromFirstArgument,
-            'RPOP'                  => $getKeyFromFirstArgument,
-            'BLPOP'                 => array($this, 'getKeyFromBlockingListCommands'),
-            'BRPOP'                 => array($this, 'getKeyFromBlockingListCommands'),
-            'LPUSH'                 => $getKeyFromFirstArgument,
-            'LPUSHX'                => $getKeyFromFirstArgument,
-            'RPUSH'                 => $getKeyFromFirstArgument,
-            'RPUSHX'                => $getKeyFromFirstArgument,
-            'LRANGE'                => $getKeyFromFirstArgument,
-            'LREM'                  => $getKeyFromFirstArgument,
-            'LSET'                  => $getKeyFromFirstArgument,
-            'LTRIM'                 => $getKeyFromFirstArgument,
-
-            /* commands operating on sets */
-            'SADD'                  => $getKeyFromFirstArgument,
-            'SCARD'                 => $getKeyFromFirstArgument,
-            'SISMEMBER'             => $getKeyFromFirstArgument,
-            'SMEMBERS'              => $getKeyFromFirstArgument,
-            'SSCAN'                 => $getKeyFromFirstArgument,
-            'SPOP'                  => $getKeyFromFirstArgument,
-            'SRANDMEMBER'           => $getKeyFromFirstArgument,
-            'SREM'                  => $getKeyFromFirstArgument,
-
-            /* commands operating on sorted sets */
-            'ZADD'                  => $getKeyFromFirstArgument,
-            'ZCARD'                 => $getKeyFromFirstArgument,
-            'ZCOUNT'                => $getKeyFromFirstArgument,
-            'ZINCRBY'               => $getKeyFromFirstArgument,
-            'ZRANGE'                => $getKeyFromFirstArgument,
-            'ZRANGEBYSCORE'         => $getKeyFromFirstArgument,
-            'ZRANK'                 => $getKeyFromFirstArgument,
-            'ZREM'                  => $getKeyFromFirstArgument,
-            'ZREMRANGEBYRANK'       => $getKeyFromFirstArgument,
-            'ZREMRANGEBYSCORE'      => $getKeyFromFirstArgument,
-            'ZREVRANGE'             => $getKeyFromFirstArgument,
-            'ZREVRANGEBYSCORE'      => $getKeyFromFirstArgument,
-            'ZREVRANK'              => $getKeyFromFirstArgument,
-            'ZSCORE'                => $getKeyFromFirstArgument,
-            'ZSCAN'                 => $getKeyFromFirstArgument,
-            'ZLEXCOUNT'             => $getKeyFromFirstArgument,
-            'ZRANGEBYLEX'           => $getKeyFromFirstArgument,
-            'ZREMRANGEBYLEX'        => $getKeyFromFirstArgument,
-
-            /* commands operating on hashes */
-            'HDEL'                  => $getKeyFromFirstArgument,
-            'HEXISTS'               => $getKeyFromFirstArgument,
-            'HGET'                  => $getKeyFromFirstArgument,
-            'HGETALL'               => $getKeyFromFirstArgument,
-            'HMGET'                 => $getKeyFromFirstArgument,
-            'HMSET'                 => $getKeyFromFirstArgument,
-            'HINCRBY'               => $getKeyFromFirstArgument,
-            'HINCRBYFLOAT'          => $getKeyFromFirstArgument,
-            'HKEYS'                 => $getKeyFromFirstArgument,
-            'HLEN'                  => $getKeyFromFirstArgument,
-            'HSET'                  => $getKeyFromFirstArgument,
-            'HSETNX'                => $getKeyFromFirstArgument,
-            'HVALS'                 => $getKeyFromFirstArgument,
-            'HSCAN'                 => $getKeyFromFirstArgument,
-
-            /* commands operating on HyperLogLog */
-            'PFADD'                 => $getKeyFromFirstArgument,
-            'PFCOUNT'               => $getKeyFromAllArguments,
-            'PFMERGE'               => $getKeyFromAllArguments,
-
-            /* scripting */
-            'EVAL'                  => array($this, 'getKeyFromScriptingCommands'),
-            'EVALSHA'               => array($this, 'getKeyFromScriptingCommands'),
-        );
-    }
-
-    /**
-     * Returns the list of IDs for the supported commands.
-     *
-     * @return array
-     */
-    public function getSupportedCommands()
-    {
-        return array_keys($this->commands);
-    }
-
-    /**
-     * Sets an handler for the specified command ID.
-     *
-     * The signature of the callback must have a single parameter
-     * of type Predis\Command\CommandInterface.
-     *
-     * When the callback argument is omitted or NULL, the previously
-     * associated handler for the specified command ID is removed.
-     *
-     * @param string $commandId The ID of the command to be handled.
-     * @param mixed  $callback  A valid callable object or NULL.
-     */
-    public function setCommandHandler($commandId, $callback = null)
-    {
-        $commandId = strtoupper($commandId);
-
-        if (!isset($callback)) {
-            unset($this->commands[$commandId]);
-
-            return;
-        }
-
-        if (!is_callable($callback)) {
-            throw new \InvalidArgumentException("Callback must be a valid callable object or NULL");
-        }
-
-        $this->commands[$commandId] = $callback;
-    }
-
     /**
-     * Extracts the key from the first argument of a command instance.
      *
-     * @param  CommandInterface $command Command instance.
-     * @return string
      */
-    protected function getKeyFromFirstArgument(CommandInterface $command)
+    public function __construct(HashGeneratorInterface $hashGenerator = null)
     {
-        return $command->getArgument(0);
-    }
-
-    /**
-     * Extracts the key from a command that can accept multiple keys ensuring
-     * that only one key is actually specified to comply with redis-cluster.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromAllArguments(CommandInterface $command)
-    {
-        $arguments = $command->getArguments();
-
-        if (count($arguments) === 1) {
-            return $arguments[0];
-        }
-    }
-
-    /**
-     * Extracts the key from a command that can accept multiple keys ensuring
-     * that only one key is actually specified to comply with redis-cluster.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromInterleavedArguments(CommandInterface $command)
-    {
-        $arguments = $command->getArguments();
-
-        if (count($arguments) === 2) {
-            return $arguments[0];
-        }
-    }
-
-    /**
-     * Extracts the key from BLPOP and BRPOP commands ensuring that only one key
-     * is actually specified to comply with redis-cluster.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromBlockingListCommands(CommandInterface $command)
-    {
-        $arguments = $command->getArguments();
-
-        if (count($arguments) === 2) {
-            return $arguments[0];
-        }
-    }
-
-    /**
-     * Extracts the key from EVAL and EVALSHA commands.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromScriptingCommands(CommandInterface $command)
-    {
-        if ($command instanceof ScriptedCommand) {
-            $keys = $command->getKeys();
-        } else {
-            $keys = array_slice($args = $command->getArguments(), 2, $args[1]);
-        }
-
-        if (count($keys) === 1) {
-            return $keys[0];
-        }
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getHash(CommandInterface $command)
-    {
-        $hash = $command->getHash();
-
-        if (!isset($hash) && isset($this->commands[$cmdID = $command->getId()])) {
-            $key = call_user_func($this->commands[$cmdID], $command);
-
-            if (isset($key)) {
-                $hash = $this->hashGenerator->hash($key);
-                $command->setHash($hash);
-            }
-        }
-
-        return $hash;
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getKeyHash($key)
-    {
-        $key = $this->extractKeyTag($key);
-        $hash = $this->hashGenerator->hash($key);
-
-        return $hash;
+        parent::__construct($hashGenerator ?: new CRC16HashGenerator());
     }
 
     /**

tests/Predis/Cluster/RedisClusterHashStrategyTest.php

@@ -275,6 +275,8 @@ class RedisClusterHashStrategyTest extends PredisTestCase
             'TTL'                   => 'keys-first',
             'PTTL'                  => 'keys-first',
             'SORT'                  => 'keys-first', // TODO
+            'DUMP'                  => 'keys-first',
+            'RESTORE'               => 'keys-first',
 
             /* commands operating on string values */
             'APPEND'                => 'keys-first',
@@ -297,6 +299,7 @@ class RedisClusterHashStrategyTest extends PredisTestCase
             'SETRANGE'              => 'keys-first',
             'STRLEN'                => 'keys-first',
             'SUBSTR'                => 'keys-first',
+            'BITOP'                 => 'keys-bitop',
             'BITCOUNT'              => 'keys-first',
 
             /* commands operating on lists */
@@ -305,8 +308,10 @@ class RedisClusterHashStrategyTest extends PredisTestCase
             'LLEN'                  => 'keys-first',
             'LPOP'                  => 'keys-first',
             'RPOP'                  => 'keys-first',
+            'RPOPLPUSH'             => 'keys-all',
             'BLPOP'                 => 'keys-blockinglist',
             'BRPOP'                 => 'keys-blockinglist',
+            'BRPOPLPUSH'            => 'keys-blockinglist',
             'LPUSH'                 => 'keys-first',
             'LPUSHX'                => 'keys-first',
             'RPUSH'                 => 'keys-first',
@@ -319,6 +324,12 @@ class RedisClusterHashStrategyTest extends PredisTestCase
             /* commands operating on sets */
             'SADD'                  => 'keys-first',
             'SCARD'                 => 'keys-first',
+            'SDIFF'                 => 'keys-all',
+            'SDIFFSTORE'            => 'keys-all',
+            'SINTER'                => 'keys-all',
+            'SINTERSTORE'           => 'keys-all',
+            'SUNION'                => 'keys-all',
+            'SUNIONSTORE'           => 'keys-all',
             'SISMEMBER'             => 'keys-first',
             'SMEMBERS'              => 'keys-first',
             'SSCAN'                 => 'keys-first',
@@ -331,6 +342,7 @@ class RedisClusterHashStrategyTest extends PredisTestCase
             'ZCARD'                 => 'keys-first',
             'ZCOUNT'                => 'keys-first',
             'ZINCRBY'               => 'keys-first',
+            'ZINTERSTORE'           => 'keys-zaggregated',
             'ZRANGE'                => 'keys-first',
             'ZRANGEBYSCORE'         => 'keys-first',
             'ZRANK'                 => 'keys-first',
@@ -341,6 +353,7 @@ class RedisClusterHashStrategyTest extends PredisTestCase
             'ZREVRANGEBYSCORE'      => 'keys-first',
             'ZREVRANK'              => 'keys-first',
             'ZSCORE'                => 'keys-first',
+            'ZUNIONSTORE'           => 'keys-zaggregated',
             'ZSCAN'                 => 'keys-first',
             'ZLEXCOUNT'             => 'keys-first',
             'ZRANGEBYLEX'           => 'keys-first',

tests/Predis/Connection/RedisClusterTest.php

@@ -427,7 +427,7 @@ class RedisClusterTest extends PredisTestCase
     /**
      * @group disconnected
      */
-    public function testDoesNotSupportKeyTags()
+    public function testSupportsKeyTags()
     {
         $profile = ServerProfile::getDefault();
 
@@ -445,8 +445,8 @@ class RedisClusterTest extends PredisTestCase
 
         $set = $profile->createCommand('set', array('{node:1001}:bar', 'foobar'));
         $get = $profile->createCommand('get', array('{node:1001}:bar'));
-        $this->assertSame($connection2, $cluster->getConnection($set));
-        $this->assertSame($connection2, $cluster->getConnection($get));
+        $this->assertSame($connection1, $cluster->getConnection($set));
+        $this->assertSame($connection1, $cluster->getConnection($get));
     }
 
     /**