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

Master nodes not discovered #338

Closed
danieleteti opened this issue May 24, 2016 · 5 comments
Closed

Master nodes not discovered #338

danieleteti opened this issue May 24, 2016 · 5 comments
Labels
redis-cluster Cluster (managed by redis-cluster)

Comments

@danieleteti
Copy link

In a redis-cluster with 3 master and 3 slave (one for each) I have that some master could become a slave because of a master fault or simply a restart.
Now, how I can discover which are the current master nodes?
I've to delete a key which starts with a particupar pattern (let's say cache:aaa*) on all the nodes.
This is what I currently do:

  public function keys_clustered($pattern) {
    $allkeys = [];
    $client = $this->get_redis(); //return the predis client
    $cmdKeys = $client->createCommand('keys', [$pattern]);

    $connections = $client->getConnection();
    if (!$connections) {
      throw new Exception("Cannot get REDIS connections in " . get_class($this));
    }
    if (count($connections) > 1) {
      foreach ($client->getConnection() as $nodeConnection) {
        $nodeKeys = $nodeConnection->executeCommand($cmdKeys);
        $allkeys = array_merge($allkeys, $nodeKeys);
      }
    } else {
      $cmdKeys = $client->createCommand('keys', [$pattern]);
      $allkeys = $client->executeCommand($cmdKeys);
    }
    return $allkeys;
  }

But this code doesn't search for actual masters. It simply uses the configured list of node in the predis constructor.

How I can get all these keys?

@nrk
Copy link
Contributor

nrk commented May 25, 2016

Hi @danieleteti,

Indeed iterating over Predis\Connection\Aggregate\RedisCluster usually doesn't return all the connections to the master nodes of a redis-managed cluster, instead you get the connection instances currently initialized by Predis (not only the ones supplied to Predis\Client::__construct() but also the ones that the client discovered and connected to).

That said, trying to delete a subset of the keys spread over the keyspace is a bit tricky with redis-cluster but still not impossible. First of all I suggest avoiding KEYS because it's a resource-intensive command, it can "block" redis when the single instance holds millions of keys and can potentially return an humongous-sized response which could even end up killing PHP due to the memory_limit directive, instead you should use SCAN which iterates the keyspace incrementally with a cursor-based approach (we have a nice abstraction in Predis for that).

This is a quick example of a function that fetches the current slots map from redis-cluster in order to extract the list of master nodes from it, then it creates a new connection for each master node and uses it to create a new client (for convenience instead of using ConnectionInterface::executeCommand()) that gets passed to a user-supplied callback:

function cluster_iterate(\Predis\ClientInterface $client, $callback)
{
    $clientClass = get_class($client);
    $clientOptions = $client->getOptions();

    $cluster = $client->getConnection();
    $connector = $cluster->getConnectionFactory();
    $slotsmap = $cluster->getSlotsMap() ?: $cluster->askSlotsMap();

    foreach (array_unique($slotsmap) as $node) {
        $nodeConnection = $connector->create($node);
        $nodeClient = new $clientClass($nodeConnection, $clientOptions);

        $callback($nodeClient);
    }
}

A script that deletes the keys matching a certain pattern on each node of the cluster would then look like this:

$clusterClient = new \Predis\Client(['tcp://127.0.0.1:7000'], ['cluster' => 'redis']);

cluster_iterate($clusterClient, function ($nodeClient) {
    $keyspace = new \Predis\Collection\Iterator\Keyspace($nodeClient, "cache:*");

    foreach ($keyspace as $key) {
        $nodeClient->del($key);
    }
});

Everything considered I think this is the best possible compromise, but this approach still requires a few considerations:

  • One could think of implementing a chunked iterator (an iterator that returns multiple values at once from the inner iterator) and wrap Predis\Collection\Iterator\Keyspace in order to use the variadic property of DEL, unfortunately it is not possible to send a single DEL with multiple keys to a node participating to redis-cluster as it would most likely end up raising a -CROSSSLOT error even when all of the specified keys live on the same node. That's how redis-cluster works.
  • It is still possible that a migration in the cluster occurs during the iteration, this will almost certainly result in -MOVED or -ASK responses being returned by Redis and the client will throw an exception. How to handle this exception is up to the implementer, one could even try to reissue the same command using $clusterClient instead of $nodeClient so that the client will discover the correct node by itself, but in this specific case with DEL you might end up with keys migrated from a node ahead of the iteration to one past the current position of the iteration (if this happens, some keys could be left undeleted in the "global" keyspace).

@nrk nrk added the redis-cluster Cluster (managed by redis-cluster) label May 25, 2016
@danieleteti
Copy link
Author

danieleteti commented May 27, 2016

Thank you for your example. Currently I finished to ask a CLUSTER NODES to redis and then create a specific predis instance with only the master nodes. I'll change KEYS to SCAN ASAP, but still I need a way to be sure that predis is talking only with master nodes.
Somehting like:

$config = [my nodes array with some masters and some slaves];
$client = new PRedis($config);
//Here I've to be sure that $client is connected only to master nodes...

With my solution I've to reimplement a microredisclient to issue a CLUSTER NODES to understand how the cluster is and the crete predis. Is there a way to let predis understand itself and let it connects only to master nodes?

Still thank you.

@nrk
Copy link
Contributor

nrk commented May 27, 2016

Why putting slaves at all in $config? The parameters array for Predis\Client must point only to master nodes in the cluster (actually you don't even strictly need to specify all of them). Moreover, when Predis requests the updated slots map from the cluster, the current implementation of askSlotsMap() discards any detail about slaves. Slaves, after all, are not really that useful to the client in this context unless you want to spread read-only operations to reduce the load on the master nodes (a feature which is currently not supported by Predis). When a master node fails, redis-cluster automatically promotes one of the slaves and this change is reflected in the output of the cluster-related commands when asking for the new configuration. Basically we don't care about slaves.

By the way if you want to get these info "manually" from a node of the cluster, I think it's easier to use CLUSTER SLOTS (even if you don't care about the slots map configuration) as the response format is way more friendly compared to that of CLUSTER NODES.

@nrk
Copy link
Contributor

nrk commented May 28, 2016

Just a quick update: after commit 922e56b (which means starting with Predis v1.1.0 when it will be released) the function cluster_iterate() from my previous comment can be reduced to the following:

function cluster_iterate(\Predis\ClientInterface $client, $callback)
{
    $clientClass = get_class($client);

    foreach ($client->getConnection() as $nodeConnection) {
        $nodeClient = new $clientClass($nodeConnection, $client->getOptions());
        $callback($nodeClient);
    }
}

Basically Predis\Connection\Aggregate\RedisCluster::getIterator() now returns all the connections to the master nodes in the cluster (even the ones not yet registered in the pool) by fetching the slots map from one of the servers.

@nrk
Copy link
Contributor

nrk commented Jun 1, 2016

Improvements have been made for v1.1.0 so I'm closing this issue, anyway feel free to post further comments on the matter if you need more clarifications.

EDIT: just pushed the last change for Predis v1.1 with ecab7e4 to make it possible to iterate each node of an underlying traversable aggregate connection simply by doing a foreach on the client:

foreach ($client as $nodeClient) {
    $nodeClient->flushdb();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
redis-cluster Cluster (managed by redis-cluster)
Development

No branches or pull requests

2 participants