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.

Sign up for GitHub

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

Jump to bottom

Adding support for RedisCluster (phpredis extension) #191

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Adding support for RedisCluster (phpredis extension) #191

wants to merge 25 commits into from

Conversation

JacobBrownAustin
Copy link

@JacobBrownAustin JacobBrownAustin commented Sep 18, 2024 •
edited
Loading

@JacobBrownAustin JacobBrownAustin mentioned this pull request Sep 18, 2024
Open
@colinmollenhour
Copy link
Owner

Thanks for the PR! I think this should probably just replace Credis_Cluster since it was never completed and would need a lot of work and having both would be confusing. Would you mind removing that, updating the README and add a basic PHPUnit test? Also, I think the constructor should probably throw an exception if the phpredis extension is not present.

@infowolfe
Copy link

@JacobBrownAustin thank you for this! Magento has been missing support for redis 6 for 4 years.

@JacobBrownAustin
Copy link
Author

Thanks for the PR! I think this should probably just replace Credis_Cluster since it was never completed and would need a lot of work and having both would be confusing. Would you mind removing that, updating the README and add a basic PHPUnit test? Also, I think the constructor should probably throw an exception if the phpredis extension is not present.

Those all sound great. I'll update this soon.

@JacobBrownAustin
Copy link
Author

@colinmollenhour , the Credis_Sentinel also uses Credis_Cluster. What should be done about that?

https://github.com/colinmollenhour/credis/blob/master/Sentinel.php#L312
https://github.com/colinmollenhour/credis/blob/master/Sentinel.php#L268

@JacobBrownAustin
Copy link
Author

@colinmollenhour , because the new class is basically just an adapter to the php redis extension, there's not much logic to test in the class itself as a unit test, so I'll add a Redis cluster to the docker compose to test it more of an integration test.

@colinmollenhour
Copy link
Owner

@colinmollenhour , the Credis_Sentinel also uses Credis_Cluster. What should be done about that?

Both of those methods are marked as deprecated so they can be removed. The new version will get a major version bump already.

@colinmollenhour
Copy link
Owner

@colinmollenhour , because the new class is basically just an adapter to the php redis extension, there's not much logic to test in the class itself as a unit test, so I'll add a Redis cluster to the docker compose to test it more of an integration test.

Sounds good, thanks!

@hostep hostep mentioned this pull request Sep 30, 2024
Open
5 tasks
@infowolfe
Copy link

@JacobBrownAustin any movement on this?

@JacobBrownAustin
Copy link
Author

@infowolfe
Sorry, I had to fix some bugs in a different project last couple of weeks, but I'm working on this again now.

@JacobBrownAustin
Credis_Cluster using RedisCluster & adding PHP 8.3 test container
2a547d5 
* Credis_Cluster class now replaced with new class that uses
  RedisCluster.
* Updated tests
  * Lots of tests skipped because of broken/missing features or
    incompatible behaviour because of Redis clusters or RedisCluster
* Adding PHP 8.3 test container to run the phpunit tests
@JacobBrownAustin
Updating Documentation for Credis_Client
1541640
@JacobBrownAustin
Updating Documentation for Credis_Client
648254e
@JacobBrownAustin
Copy link
Author

@colinmollenhour @infowolfe
Okay, I've updated my pull request.

It now replaces the previously @deprecated Credis_Cluster class.

I'm running the same tests as CredisTest, but I've had to skip 25 of them due to incompatibilities, known issues, and some bugs I've found with RedisCluster. A lot of them are due to the fact that some of the methods require an additional parameter to specify which node in the cluster to run on like flushDb and save. We could write adapters for this methods to get the most recent cluster information and then run the same command on all nodes of the cluster, like flushDb for example; but this is beyond the scope of my project as I'm not currently needing any of these methods.
I've added PHP 8.4 testing container, in addition to the existing one which is PHP 7.3.
I've noticed that this project still says it supports PHP 5.4, but there's no container that is actually testing this, so I have no idea if that actually works on PHP 5.4.

I've updated the documentation for this new class.

And yeah, I added an Exception thrown in the constructor if the RedisCluster class from the 'redis' extension is missing.

JacobBrownAustin added a commit to JacobBrownAustin/php-redis-session-abstract that referenced this pull request Oct 15, 2024
@JacobBrownAustin
Updated for change in colinmollenhour/credis#191
6c5d13d
@colinmollenhour
Copy link
Owner

We can drop PHP 5.4, I think 7.4 should be the lowest to support. I'll try to take a closer look soon. @infowolfe if you have a chance to do some code review and test that would help a lot as well!

@JacobBrownAustin
Copy link
Author

@colinmollenhour What's the status on accepting this pull request?
I see it is now conflicting with Cluster.php.

@colinmollenhour
Copy link
Owner

colinmollenhour commented Jan 28, 2025 •
edited
Loading

I fixed merge conflicts and merged the latest to fix code style checks. However, there are a lot of broken tests. If you can get them working we should be good to go.

@JacobBrownAustin
Copy link
Author

Hi @colinmollenhour , I need your opinion on something.
In the RedisCluster from the "redis" extension for PHP, the API of FlushAll and FlushDB has changed as the first parameter is now either a key (so that it can look up the slot) or an array of host & port so that it can look up which node to run the command against. Because this breaks the API, I was thinking of changing the way this behaves in the Credis_Cluster class so that the "flushDb" method (and similar methods) will get the list of all current nodes, and then run the same command against all of the nodes. For example, flushDb would get the current list of nodes using "CLUSTER NODES" or "CLUST SLOTS" command, then for each of them, run FlushDB against that host/port. Then we could add a new methods to Credis_Cluster class which would be like "flushDbForNode" which allows specifying the specific node to run against using either key (which maps to slot) or host/port.

What do you think?

@JacobBrownAustin
Copy link
Author

Also, I've been working on the tests so that the class now spawns the servers and assembles the cluster by itself without needing Docker Compose. (I didn't realize that your tests in Git Hub CI weren't using the same docker compose for running tests locally.) In addition, I found some behaviour which was wrong some I'm making some corrections in a few places.

@colinmollenhour
Copy link
Owner

@JacobBrownAustin Your proposal sounds like great solution!

@JacobBrownAustin
Copy link
Author

Okay. I'll make those changes soon. Also, there's a few tests that I skipped which I saw recently that can be modified for cluster to run. (Some of the queries being tested can run if forced onto the same cluster.)

@JacobBrownAustin
Copy link
Author

FYI, I believe the tests should pass now in GitHub, but I don't know how to trigger it and I'm guessing I just don't have access to trigger it.

@colinmollenhour
Copy link
Owner

I changed the settings, so you should be able to run the workflow now without approval.

@JacobBrownAustin
Copy link
Author

Wahoo, finally got them all the pass, and not just 5/6ths.
I'm going to try to unskip a few tests that are currently marked skipped that I think I know how to work in Redis Cluster. After that, I think it should be ready.

@colinmollenhour
Copy link
Owner

Killing it! Thanks for your work on this. Will wait for final review until you report you are done.
Have a great weekend!

@JacobBrownAustin
adding few node-specific methods to Credis_Cluster, updated some tests,
700027a 
and also switched away from append-only mode for the database
JacobBrownAustin
JacobBrownAustin commented Feb 7, 2025
View reviewed changes
Cluster.php
*
* @deprecated
* Note: RedisCluster currently has limitations like not supporting pipeline or multi.
* Note: Many methods require an additional parameter to specify which node to run on, and only run on that node,
Copy link
Author

Choose a reason for hiding this comment

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

I need to update the comments here after the changes that I've made in last couple of days.

JacobBrownAustin
JacobBrownAustin commented Feb 7, 2025
View reviewed changes
README.markdown
echo $cluster->get('key').PHP_EOL;
```
* RedisCluster currently has limitations like not supporting pipeline or multi. This may be added in the future. See [here](https://github.com/phpredis/phpredis/blob/develop/cluster.md) for details.
* Many methods require an additional parameter to specify which node to run on, and only run on that node, such as save(), flushDB(), ping(), and scan().
Copy link
Author

Choose a reason for hiding this comment

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

Need to update this after changes from last couple of days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JacobBrownAustin @colinmollenhour @infowolfe