plugin-QueuedTracking
plugin-QueuedTracking copied to clipboard
Plugin ready to work with clusters?
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
Any chance you could try to patch the plugin and issue a PR? In case there is any experience with PHP.
@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 Are you on a Kubernetes / Swarm cluster ?
@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
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.
@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 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
@toredash I actually don't know what I did exactly. I assume I replaced the
Redis()
class with theRedisCluster()
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);
}
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?
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.