Skip to content

Commit

Permalink
[Cache] Fix perf when using RedisCluster by reducing roundtrips to th…
Browse files Browse the repository at this point in the history
…e servers

This is slimmed down version of: #28269 _(many of the fixes here are already part of 3.4)_

Adds:
- Test coverage for Predis with RedisCluster
- Removes usage of key versioning when on RedisCluster, besides performance aspect of that simplify / aligning clear() handling across cases
  • Loading branch information
nicolas-grekas authored and andrerom committed Mar 11, 2019
1 parent 21d0197 commit 2d75184
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 22 deletions.
@@ -0,0 +1,28 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Cache\Tests\Adapter;

class PredisRedisClusterAdapterTest extends AbstractRedisAdapterTest
{
public static function setupBeforeClass()
{
if (!$hosts = getenv('REDIS_CLUSTER_HOSTS')) {
self::markTestSkipped('REDIS_CLUSTER_HOSTS env var is not defined.');
}
self::$redis = new \Predis\Client(explode(' ', $hosts), array('cluster' => 'redis'));
}

public static function tearDownAfterClass()
{
self::$redis = null;
}
}
40 changes: 18 additions & 22 deletions src/Symfony/Component/Cache/Traits/RedisTrait.php
Expand Up @@ -12,7 +12,6 @@
namespace Symfony\Component\Cache\Traits;

use Predis\Connection\Aggregate\ClusterInterface;
use Predis\Connection\Aggregate\PredisCluster;
use Predis\Connection\Aggregate\RedisCluster;
use Predis\Connection\Factory;
use Predis\Response\Status;
Expand Down Expand Up @@ -48,9 +47,7 @@ private function init($redisClient, $namespace = '', $defaultLifetime = 0)
if (preg_match('#[^-+_.A-Za-z0-9]#', $namespace, $match)) {
throw new InvalidArgumentException(sprintf('RedisAdapter namespace contains "%s" but only characters in [-+_.A-Za-z0-9] are allowed.', $match[0]));
}
if ($redisClient instanceof \RedisCluster) {
$this->enableVersioning();
} elseif (!$redisClient instanceof \Redis && !$redisClient instanceof \RedisArray && !$redisClient instanceof \Predis\Client && !$redisClient instanceof RedisProxy) {
if (!$redisClient instanceof \Redis && !$redisClient instanceof \RedisArray && !$redisClient instanceof \RedisCluster && !$redisClient instanceof \Predis\Client && !$redisClient instanceof RedisProxy) {
throw new InvalidArgumentException(sprintf('%s() expects parameter 1 to be Redis, RedisArray, RedisCluster or Predis\Client, %s given', __METHOD__, \is_object($redisClient) ? \get_class($redisClient) : \gettype($redisClient)));
}
$this->redis = $redisClient;
Expand Down Expand Up @@ -207,9 +204,6 @@ protected function doHave($id)
*/
protected function doClear($namespace)
{
// When using a native Redis cluster, clearing the cache is done by versioning in AbstractTrait::clear().
// This means old keys are not really removed until they expire and may need garbage collection.

$cleared = true;
$hosts = [$this->redis];
$evalArgs = [[$namespace], 0];
Expand All @@ -218,21 +212,23 @@ protected function doClear($namespace)
$evalArgs = [0, $namespace];

$connection = $this->redis->getConnection();
if ($connection instanceof PredisCluster) {
if ($connection instanceof ClusterInterface && $connection instanceof \Traversable) {
$hosts = [];
foreach ($connection as $c) {
$hosts[] = new \Predis\Client($c);
}
} elseif ($connection instanceof RedisCluster) {
return false;
}
} elseif ($this->redis instanceof \RedisArray) {
$hosts = [];
foreach ($this->redis->_hosts() as $host) {
$hosts[] = $this->redis->_instance($host);
}
} elseif ($this->redis instanceof \RedisCluster) {
return false;
$hosts = [];
foreach ($this->redis->_masters() as $host) {
$hosts[] = $h = new \Redis();
$h->connect($host[0], $host[1]);
}
}
foreach ($hosts as $host) {
if (!isset($namespace[0])) {
Expand All @@ -259,7 +255,7 @@ protected function doClear($namespace)
$keys = $keys[1];
}
if ($keys) {
$host->del($keys);
$this->doDelete($keys);
}
} while ($cursor = (int) $cursor);
}
Expand Down Expand Up @@ -331,7 +327,16 @@ private function pipeline(\Closure $generator)
{
$ids = [];

if ($this->redis instanceof \Predis\Client && !$this->redis->getConnection() instanceof ClusterInterface) {
if ($this->redis instanceof \RedisCluster || ($this->redis instanceof \Predis\Client && $this->redis->getConnection() instanceof RedisCluster)) {
// phpredis & predis don't support pipelining with RedisCluster
// see https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#pipelining
// see https://github.com/nrk/predis/issues/267#issuecomment-123781423
$results = [];
foreach ($generator() as $command => $args) {
$results[] = \call_user_func_array(array($this->redis, $command), $args);
$ids[] = $args[0];
}
} elseif ($this->redis instanceof \Predis\Client) {
$results = $this->redis->pipeline(function ($redis) use ($generator, &$ids) {
foreach ($generator() as $command => $args) {
\call_user_func_array([$redis, $command], $args);
Expand All @@ -355,15 +360,6 @@ private function pipeline(\Closure $generator)
foreach ($results as $k => list($h, $c)) {
$results[$k] = $connections[$h][$c];
}
} elseif ($this->redis instanceof \RedisCluster || ($this->redis instanceof \Predis\Client && $this->redis->getConnection() instanceof ClusterInterface)) {
// phpredis & predis don't support pipelining with RedisCluster
// see https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#pipelining
// see https://github.com/nrk/predis/issues/267#issuecomment-123781423
$results = [];
foreach ($generator() as $command => $args) {
$results[] = \call_user_func_array([$this->redis, $command], $args);
$ids[] = $args[0];
}
} else {
$this->redis->multi(\Redis::PIPELINE);
foreach ($generator() as $command => $args) {
Expand Down

0 comments on commit 2d75184

Please sign in to comment.