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

add redis cluster capability #262

Conversation

haristku
Copy link
Contributor

Description:

breaking down the previous PR to specific part

  • add redis cluster capability

@AltamashShaikh

i already solved the issue on the already checked "Enable Redis Sentinel" using this line, but i need your suggestion on whether do i need to add a version migration "Updates" to run the sql script below or not?

INSERT INTO matomo_plugin_setting (plugin_name, setting_name, setting_value) 
SELECT * FROM (
    SELECT plugin_name, 'useWhatRedisBackendType' AS setting_name, CASE WHEN setting_value=1 THEN 2 ELSE 1 END AS setting_value FROM matomo_plugin_setting 
    WHERE plugin_name='QueuedTracking' AND setting_name='useSentinelBackend'
) AS tmp 
WHERE NOT EXISTS (
    SELECT * FROM matomo_plugin_setting
    WHERE 
        (plugin_name='QueuedTracking' AND setting_name='useWhatRedisBackendType') 
        OR (plugin_name='QueuedTracking' AND setting_name='backend' AND setting_value!='redis')
);

ty.

SystemSettings.php Outdated Show resolved Hide resolved
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. I only had one suggested change. Sadly, I don't have a clustered environment to test with.

SystemSettings.php Outdated Show resolved Hide resolved
@snake14
Copy link
Contributor

snake14 commented Oct 14, 2024

Description:

breaking down the previous PR to specific part

  • add redis cluster capability

@AltamashShaikh

i already solved the issue on the already checked "Enable Redis Sentinel" using this line, but i need your suggestion on whether do i need to add a version migration "Updates" to run the sql script below or not?

INSERT INTO matomo_plugin_setting (plugin_name, setting_name, setting_value) 
SELECT * FROM (
    SELECT plugin_name, 'useWhatRedisBackendType' AS setting_name, CASE WHEN setting_value=1 THEN 2 ELSE 1 END AS setting_value FROM matomo_plugin_setting 
    WHERE plugin_name='QueuedTracking' AND setting_name='useSentinelBackend'
) AS tmp 
WHERE NOT EXISTS (
    SELECT * FROM matomo_plugin_setting
    WHERE 
        (plugin_name='QueuedTracking' AND setting_name='useWhatRedisBackendType') 
        OR (plugin_name='QueuedTracking' AND setting_name='backend' AND setting_value!='redis')
);

ty.

@haristku That seems a bit more complicated than necessary. Please see my review comment: https://github.com/matomo-org/plugin-QueuedTracking/pull/262/files#r1798722794

@haristku
Copy link
Contributor Author

haristku commented Oct 14, 2024

Looks pretty good. I only had one suggested change. Sadly, I don't have a clustered environment to test with.

if you want to consider to test the clustered redis, simply:

  1. download it from here
  2. extract it
  3. change directory to utils/create-cluster
  4. run the command ./create-cluster create
  5. "once" created, we just need to start it with this command ./create-cluster start
  6. ./create-cluster stop to stop the cluster, and we can restart it without having to recreate it

the cluster master will bind on 127.0.0.1:30001, 127.0.0.1:30002, 127.0.0.1:30003.

if we want to change the port, run this echo "PORT=7000" > config.sh, make sure to stop the cluster, recreate & start.

and in the matomo web settings we can input:
Cluster in "Redis Type" section,
127.0.0.1,127.0.0.1,127.0.0.1 in "Redis host or unix socket" section,
30001,30002,30003 in "Redis port" section,

ty.

@haristku
Copy link
Contributor Author

@AltamashShaikh @snake14

config migration done..
ty.

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good 👍 I tested the migration, but I haven't had a chance to test with a cluster yet.

Copy link
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has good amount of changes, will release v5.1.0 of the plugin and update the migration script accordingly.

@haristku Thank you very much for all of your contributions :)

@AltamashShaikh AltamashShaikh merged commit 9ddbab2 into matomo-org:5.x-dev Oct 21, 2024
5 checks passed
@haristku
Copy link
Contributor Author

@AltamashShaikh @snake14

you're welcome, i'm happy to be able to contribute. hopefully it can be useful. 👍

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

Successfully merging this pull request may close these issues.

3 participants