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

Plugin ready to work with clusters? #66

Closed
Tyrdall opened this issue Feb 23, 2018 · 12 comments
Closed

Plugin ready to work with clusters? #66

Tyrdall opened this issue Feb 23, 2018 · 12 comments

Comments

@Tyrdall
Copy link

Tyrdall commented Feb 23, 2018

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
@tsteur
Copy link
Member

tsteur commented Feb 23, 2018

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

@Tyrdall
Copy link
Author

Tyrdall commented Feb 24, 2018

@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 ;)

@crazy-max
Copy link

@Tyrdall Are you on a Kubernetes / Swarm cluster ?

@Tyrdall
Copy link
Author

Tyrdall commented Feb 28, 2018

@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

@tsteur
Copy link
Member

tsteur commented Feb 28, 2018

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.

@toredash
Copy link
Contributor

toredash commented Dec 6, 2019

@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

@Tyrdall
Copy link
Author

Tyrdall commented Dec 6, 2019

@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
Copy link
Contributor

toredash commented Dec 6, 2019

@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);
         }

@kfogle
Copy link

kfogle commented Aug 6, 2020

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?

@tsteur
Copy link
Member

tsteur commented Aug 6, 2020

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.

@haristku
Copy link
Contributor

haristku commented Oct 18, 2024

@snake14
Copy link
Contributor

snake14 commented Oct 22, 2024

Thanks to @haristku , this was released as part of 5.1.0 (#262)

@snake14 snake14 closed this as completed Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants