Adding support for RedisCluster (phpredis extension)

[JacobBrownAustin]

related pull request:
colinmollenhour/php-redis-session-abstract#62

[colinmollenhour]

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]

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

[JacobBrownAustin]

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]

@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]

@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]

@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]

@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!

[infowolfe]

@JacobBrownAustin any movement on this?

[JacobBrownAustin]

@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]

@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.

[colinmollenhour]

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]

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

[colinmollenhour]

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]

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]

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.

[JacobBrownAustin]

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]

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]

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

[JacobBrownAustin]

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]

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

[JacobBrownAustin]

@colinmollenhour , Okay, I'm done with the things that I had planned on adding/fixing. Let me know what you think.

[colinmollenhour]

Looks great! I've not inspected the Credis_Cluster code deeply or tested it manually so merging it is not a full endorsement, but the API looks good, the test coverage is good and the tests all pass!

Do you want me to go ahead and tag a release for this or should I wait until you've fully utilized it in the session module?

[JacobBrownAustin]

Great thanks.
The pull request colinmollenhour/php-redis-session-abstract#62 shouldn't need any changes after the recent changes heere, but I should probably retest it anyways.

[JacobBrownAustin]

@colinmollenhour , I've tested this against colinmollenhour/php-redis-session-abstract#62 and everything is working. I'm ready for both of them to be released whenever you are.