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

feat: Provide support for RedisCluster Connections #9239

Open
wants to merge 1 commit into
base: 4.6
Choose a base branch
from

Conversation

kchason
Copy link

@kchason kchason commented Oct 27, 2024

Description

Add support for TLS-encrypted connections to Redis clusters using the RedisCluster object as an option for the RedisHandler cache provider. This is particularly helpful for environments such as AWS' Elasticache Serverless which uses clustered connections.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr ddevsr added tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. 4.6 labels Oct 27, 2024
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I'm sorry, but I'm afraid I can't accept these changes. If you want to add support for RedisCluster, you should create a separate handler.

There are too many differences.

  • With RedisCluster we should have the possibility to define many nodes.
  • deleteMatching() method should also scan all the nodes and take into account hash slots when calling del().
  • clean() method should iterate over every node.
  • getCacheInfo() method should iterate over every node.

As an aside: I see that you have implemented support for only one node, so why not just use the Redis class? ElastiCache for Redis allows you to set up different modes, also without clustering support.

@michalsn michalsn added the needs rework Changes requested by reviewer that are still pending label Nov 17, 2024
@damian-romanowski
Copy link

I’m interested in this. Got a few CI apps on K8s that would probably benefit of using my replicated Redis cluster instead of single nodes.

But I agree with @michalsn about the separate handler and node iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 docs needed Pull requests needing documentation write-ups and/or revisions. needs rework Changes requested by reviewer that are still pending tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants