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

Cache::flush does not work with redis clusters #20406

Closed
Dadibom opened this issue Aug 3, 2017 · 15 comments
Closed

Cache::flush does not work with redis clusters #20406

Dadibom opened this issue Aug 3, 2017 · 15 comments
Assignees
Labels

Comments

@Dadibom
Copy link

Dadibom commented Aug 3, 2017

  • Laravel Version: 5.4.31
  • PHP Version: 7.1
  • Database Driver & Version: phpredis (3.1.1-1), cluster using redis 3.2.9

Description:

(1/1) ErrorException RedisCluster::flushdb() expects exactly 1 parameter, 0 given

Illuminate\Cache\RedisStore::flush does not accept any parameters and will not pass anything in the call to $this->connection()->flushdb

Steps To Reproduce:

Set cache driver to redis, set it to use a cluster, call \Cache::flush(0 /* or whatever, won't be passed anyway */);

@Dadibom Dadibom changed the title Flush does not work with redis clusters Cache::flush does not work with redis clusters Aug 3, 2017
@themsaid
Copy link
Member

themsaid commented Aug 3, 2017

@alibo, do you have any idea how to solve this?

@alibo
Copy link
Contributor

alibo commented Aug 5, 2017

@Dadibom @themsaid PhpRedis, unlike Predis, needs a key to execute this command:

In the case of all commands which need to be directed at a node, the calling convention is identical to the Redis call, except that they require an additional (first) argument in order to deliver the command. Following is a list of each of these commands:

  1. SAVE
  2. BGSAVE
  3. FLUSHDB
  4. FLUSHALL
  5. DBSIZE
  6. BGREWRITEAOF
  7. LASTSAVE
  8. INFO
  9. CLIENT
  10. CLUSTER
  11. CONFIG
  12. PUBSUB
  13. SLOWLOG
  14. RANDOMKEY
  15. PING

https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands

    public function testDBSize() {
        for ($i = 0; $i < 10; $i++) {
            $str_key = "key:$i";
            $this->assertTrue($this->redis->flushdb($str_key));
            $this->redis->set($str_key, "val:$i");
            $this->assertEquals(1, $this->redis->dbsize($str_key));
        }
    }

https://github.com/phpredis/phpredis/blob/master/tests/RedisClusterTest.php#L113

If you connect to one of master nodes and run flushdb command, only data stored in that node will be removed and the rest of the data in other master nodes will remain.

In addition, if you use Predis, it throws an exception:

>>> Cache::store('redis')->flush();
Predis\NotSupportedException with message 'Cannot use 'FLUSHDB' with redis-cluster.'

As it doesn't remove the whole data stored in all master nodes and Predis doesn't support it, I think we can throw an exception for Cache::flush() and PhpRedisClusterConnection::flushdb($key = null) if the client is Redis Cluster.

Also, another option is listing all master nodes and executing flushdb command on all of them which I don't recommend! :D

@themsaid
Copy link
Member

themsaid commented Aug 5, 2017

@alibo any idea how to list all the masters and run flushdb on each? Would it work for both phpredis and predis?

@alibo
Copy link
Contributor

alibo commented Aug 6, 2017

@themsaid

For predis:

$flushDbCommand = new \Predis\Command\ServerFlushDatabase();

foreach ($this->connection()->getConnection() as $node) { 
    $node->executeCommand($flushDbCommand); 
}

more info: predis/predis#354

For phpredis:

foreach ($this->connection()->_masters() as [$host, $port]) {
    $redis = new Redis();
    $redis->connect($host, $port);
    $redis->flushDb();
}

more info: https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands

@tillkruss
Copy link
Contributor

@themsaid: I still think that "flushdb" is quite dangerous to clear the cache. Especially since Redis cluster has only one database. If you also use Redis for queues or sessions than "cache:clear" will delete those.

I'm not sure what the right solution is. A cache key prefix, cache tags, or something similar.

@Dadibom
Copy link
Author

Dadibom commented Jan 11, 2018

that's the intended behaviour of flushdb, there is no difference in the risk between cluster and single node setups.

@tillkruss
Copy link
Contributor

Intended behaviour or not, it's bad UX and can lead to data loss.

@laravel laravel deleted a comment from Dadibom Jan 11, 2018
@Dadibom
Copy link
Author

Dadibom commented Jan 11, 2018

Listen mate my point is that you don't know what developers use their caches for. A lot of people use their cache servers as pure caches, in which case a full clear can be done without losing anything important (since it's just cache). And on top of that - this function is in the docs and work with single node redis instances, not having it work on clusters to protect the developers doesn't really make sense

@Dadibom
Copy link
Author

Dadibom commented Jan 16, 2018

There's a cache prefix by default, right? How about scanning for all keys that start with the cache prefix and deleting those?

@meden
Copy link

meden commented Feb 5, 2018

Any news on that? Any workaround or advice? We use an automated script to deploy and clear the cache, but this issue complicates things quite a lot... Thanks

@taylorotwell
Copy link
Member

This bug doesn't exist anymore.

@krvajal
Copy link

krvajal commented May 18, 2020

Which version has the fix?

@NahedSamir
Copy link

Scan Or Del is a command for a single redis node.If you do want to use it in the cluster, first get nodes list in the cluster, and run a scan or del for each node.

Read this article for more info [https://qiita.com/mangano-ito/items/92e8a6b434c4b79b790c]

For laravel, to get each connection or node you can use
Redis::client()->getClientFor("127.0.0.1:6379");
then you can delete or scan in each connection.

@bernardwiesner
Copy link
Contributor

This bug doesn't exist anymore.

I am still having this issue with laravel 8.3 and predis.

@tillkruss
Copy link
Contributor

@bernardwiesner: Can you submit a PR to PhpRedisClusterConnection and tag me in it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants