plugin-QueuedTracking icon indicating copy to clipboard operation
plugin-QueuedTracking copied to clipboard

Plugin ready to work with clusters?

Open Tyrdall opened this issue 7 years ago • 10 comments

I've tested your plugin using a standalone server (works!) and a cluster. Unfortunately your plugin doesn't seem to be able to handle a cluster configuration.

Output of ./console queuedtracking:test:

Performing some tests:
Redis is connected: 1
Connection works in general
Success for method set(testKey, value)
Success for method setnx(testnxkey, value)

[RedisException]               
MOVED 8234 cluster-server-ip:6379
                             

Tyrdall avatar Feb 23 '18 10:02 Tyrdall

Any chance you could try to patch the plugin and issue a PR? In case there is any experience with PHP.

tsteur avatar Feb 23 '18 19:02 tsteur

@tsteur I did, but I don't have any experience in PHP. My fork is actually working, but this is nothing which should be merged into an official repo ;)

Tyrdall avatar Feb 24 '18 06:02 Tyrdall

@Tyrdall Are you on a Kubernetes / Swarm cluster ?

crazy-max avatar Feb 28 '18 14:02 crazy-max

@crazy-max I'm currently working on a Kubernetes cluster, but I was referring to: https://redis.io/topics/cluster-spec

In my amateurish custom solution I replaced Redis() with RedisCluster() and, of course, made some other adjustments to make it work.

Reference: https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#readme

Tyrdall avatar Feb 28 '18 15:02 Tyrdall

I reckon this could be added as a feature similar to sentinel in https://github.com/matomo-org/plugin-QueuedTracking/blob/3.1.0/SystemSettings.php#L61. So someone could enable Cluster via a setting and then define multiple hosts. If enabled, we would use RedisCluster here: https://github.com/matomo-org/plugin-QueuedTracking/blob/3.1.0/Queue/Backend/Redis.php#L235 (or add entire new backend here https://github.com/matomo-org/plugin-QueuedTracking/tree/3.1.0/Queue/Backend which inherits from the regular Redis class).

I reckon we would recommend to configure timeout etc via php.ini if someone needs that:

redis.clusters.timeout = "mycluster=5"
redis.clusters.read_timeout = "mycluster=10"

Instead of having to configure the hosts via the system settings we could also let users define it via php.ini like this:

# In redis.ini
redis.clusters.seeds = "mycluster[]=localhost:7000&test[]=localhost:7001"
redis.clusters.timeout = "mycluster=5"
redis.clusters.read_timeout = "mycluster=10"

and then there would be only a system setting to enter the cluster name, eg mycluster but I reckon it would be better to define the hosts in the system settings to guarantee all servers access the same redis etc.

tsteur avatar Feb 28 '18 18:02 tsteur

@Tyrdall

Could you please post the changed you did, to make this work with redis cluster ? I'm having a hard time to understand how this can be adopted for redis cluster

Thanks

toredash avatar Dec 06 '19 11:12 toredash

@toredash I actually don't know what I did exactly. I assume I replaced the Redis() class with the RedisCluster() class and adjusted settings and whatsoever.

https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#readme

Tyrdall avatar Dec 06 '19 13:12 Tyrdall

@toredash I actually don't know what I did exactly. I assume I replaced the Redis() class with the RedisCluster() class and adjusted settings and whatsoever.

https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#readme

I looked at your forked repo and figured it out.

For others, this is the changes I did to enable Redis Cluster support:

diff --git a/assets/var/www/plugins/QueuedTracking/Queue/Backend/Redis.php b/assets/var/www/plugins/QueuedTracking/Queue/Backend/Redis.php
index f1b2b86..15b1ef7 100644
--- a/assets/var/www/plugins/QueuedTracking/Queue/Backend/Redis.php
+++ b/assets/var/www/plugins/QueuedTracking/Queue/Backend/Redis.php
@@ -260,16 +260,16 @@ end';
 
     protected function connect()
     {
-        $this->redis = new \Redis();
-        $success = $this->redis->connect($this->host, $this->port, $this->timeout, null, 100);
+        $this->redis = new \RedisCluster(NULL, Array("$this->host:$this->port"), $this->timeout);
+        $success = $this->redis->info("$this->host:$this->port");
 
         if ($success && !empty($this->password)) {
             $success = $this->redis->auth($this->password);
         }
 
         if (!empty($this->database) || 0 === $this->database) {
+            # redis cluster does not support databases, aka.
+            # SELECT is not supported 
+            return $success;
+            # $this->redis->select($this->database);
 -            $this->redis->select($this->database);
         }

toredash avatar Dec 06 '19 14:12 toredash

Unfortunately, the workaround provided by @toredash doesn't work for me. The following error is returned when executing console queuedtracking:process [RedisClusterException] Couldn't map cluster keyspace using any provided seed

Can we get an update for the plugin to support Redis Cluster?

kfogle avatar Aug 06 '20 12:08 kfogle

If someone has some PHP skills and could add this feature I'd be happy to review. Unfortunately I don't have any experience with RedisCluster myself and don't have one running either.

tsteur avatar Aug 06 '20 20:08 tsteur