Skip to content

[Messenger] Support Redis Cluster #40155

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

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

nesk
Copy link
Contributor

@nesk nesk commented Feb 11, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #38264
License MIT
Doc PR symfony/symfony-docs#14956

This PR brings support for Redis Cluster in the Messenger component:

  • The first commit Support RedisCluster instance allows to pass a RedisCluster object when instanciating the Connection class, which brings support for Redis Cluster without any friction.
  • The second commit Support multiple hosts DSN for Redis Cluster is more opiniated and brings a DSN format to configure a Redis Cluster from config/packages/messenger.yaml.

Instanciating Connection with a RedisCluster object:

$redis = new \RedisCluster(null, ['host-01:6379', 'host-02:6379', 'host-03:6379', 'host-04:6379']);
$connection = new Connection([], [], [], $redis);

Configuring a Redis Cluster from YAML:

// config/packages/messenger.yaml

framework:
    messenger:
            metadata:
                default: 'redis://host-01:6379,redis://host-02:6379,redis://host-03:6379'
                lazy: 'redis://host-01:6379?lazy=1,redis://host-02:6379,redis://host-03:6379'

                # Configuration will be `lazy = true` and `auto_setup = true`
                multipleConfig: 'redis://host-01:6379?lazy=1&auto_setup=false,redis://host-02:6379,redis://host-03:6379?auto_setup=true'

This format allows to define multiple hosts for a Redis Cluster and still contains valid URLs. Custom configuration is still supported, it can be specified on only one of the URLs in the DSN (see lazy above). If the user provides multiple configurations on different URLs, they are simply merged with the following code and if an option is defined multiple times then the latest takes precedence (see multipleConfig above).

I understand the way the DSN is handled could not suit you. Please, if you close this PR only for the DSN part, just tell me and I will make a new PR with only the first commit.

@nesk nesk force-pushed the messenger-redis-cluster branch 3 times, most recently from 8d6de3a to 35fa491 Compare February 12, 2021 12:41
nesk added a commit to nesk/redis-messenger that referenced this pull request Feb 12, 2021
This is a temporary fix until symfony/symfony#40155 is merged
@nicolas-grekas nicolas-grekas added this to the 5.x milestone Feb 16, 2021
@nesk nesk force-pushed the messenger-redis-cluster branch 2 times, most recently from 139e9cc to 85889d3 Compare March 2, 2021 17:12
@nesk
Copy link
Contributor Author

nesk commented Mar 2, 2021

I have rebased the code on the latest commits of the 5.x branch and resolved all the conversations.

nesk added a commit to nesk/redis-messenger that referenced this pull request Mar 4, 2021
This is a temporary fix until symfony/symfony#40155 is merged
@nesk nesk requested a review from stof March 29, 2021 13:44
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Nyholm
Copy link
Member

Nyholm commented Apr 2, 2021

Hey.

I really like this feature. Well done.
If you look at the Cache component and the RedisAdapter (and trait) you see that it already supports clusters and multiple hosts in DSN. I suggest we use the same format. See https://symfony.com/doc/current/components/cache/adapters/redis_adapter.html

    'redis:?host[localhost]&host[localhost:6379]&host[/var/run/redis.sock:]&auth=my-password&redis_cluster=1'

Im not saying your format is better or worse, Im just saying we should be consistent.

Comment on lines +36 to +39
if (!$this->ready) {
$this->redis = $this->initializer->__invoke($this->redis);
$this->ready = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Im missing something, but can you tell me why this was changed?

Copy link
Member

Choose a reason for hiding this comment

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

that's because the signature of the initializer has changed (this is fine because this is internal)

@nicolas-grekas nicolas-grekas force-pushed the messenger-redis-cluster branch from 85889d3 to 04530fb Compare April 13, 2021 17:00
@nicolas-grekas
Copy link
Member

Thank you @nesk.

@nicolas-grekas nicolas-grekas merged commit 87d031c into symfony:5.x Apr 13, 2021
@fabpot fabpot mentioned this pull request Apr 18, 2021
xabbuh added a commit to symfony/symfony-docs that referenced this pull request May 7, 2021
This PR was merged into the 5.3-dev branch.

Discussion
----------

[Messenger] Document DSN format for Redis Cluster

Document Redis Cluster support for the Messenger component.

Related to symfony/symfony#40155

Commits
-------

243d84b [Messenger] Document DSN format for Redis Cluster
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.

[Messenger] Redis in cluster mode or custom instance
6 participants