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 configuration for Redis cluster support #166

Merged
merged 3 commits into from
Apr 22, 2024
Merged

Add configuration for Redis cluster support #166

merged 3 commits into from
Apr 22, 2024

Conversation

MartyHimmel
Copy link
Contributor

This adds a configuration setting and check to allow Redis in cluster mode (see issue #160). Adding REVERB_SCALING_CLUSTER_ENABLED=true to the .env file will change the config used from database.redis.default to database.redis.clusters.default.0. Like the horizontal scaling feature, this is off by default.

@taylorotwell
Copy link
Member

taylorotwell commented Apr 19, 2024

Hey @MartyHimmel - I would maybe take a slightly different approach here and instead let you actually specify the entire Redis connection information if you want.

Something like:

'scaling' => [
    'enabled' => env('REVERB_SCALING_ENABLED', false),
    'channel' => env('REVERB_SCALING_CHANNEL', 'reverb'),
    'server' => [
          'url' => env('REDIS_URL'),
          'host' => env('REDIS_HOST', '127.0.0.1'),
          'username' => env('REDIS_USERNAME'),
          'password' => env('REDIS_PASSWORD'),
          'port' => env('REDIS_PORT', '6379'),
          'database' => env('REDIS_DB', '0'),
    ],
],

Then pass that info into the PubSub class as an array. It's a bit more verbose on the configuration side but I think more flexible.

@MartyHimmel
Copy link
Contributor Author

Thanks for the direction @taylorotwell! This is my first contribution to any part of Laravel, so I was trying to keep it minimal. 😅

The updated config is working well except for the failing Laravel 10 test. I'll push another update as soon as I figure out what's going on there.

@MartyHimmel
Copy link
Contributor Author

Digging into that a bit more, it seems like the failing test above is unrelated (appears to be happening on the main branch, too). Is there anything else I need to do with this?

@@ -39,6 +39,14 @@
'scaling' => [
'enabled' => env('REVERB_SCALING_ENABLED', false),
'channel' => env('REVERB_SCALING_CHANNEL', 'reverb'),
'server' => [
'url' => env('REDIS_URL'),
Copy link
Collaborator

@joedixon joedixon Apr 22, 2024

Choose a reason for hiding this comment

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

I think we can remove the url key since we don't use it?

'server' => [
'url' => env('REDIS_URL'),
'host' => env('REDIS_HOST', '127.0.0.1'),
'username' => env('REDIS_USERNAME'),
Copy link
Collaborator

@joedixon joedixon Apr 22, 2024

Choose a reason for hiding this comment

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

We also don't appear to use username.

@MartyHimmel
Copy link
Contributor Author

@joedixon Done and done.

@joedixon
Copy link
Collaborator

Thanks @MartyHimmel - just want to confirm, this configuration works with your cluster?

@MartyHimmel
Copy link
Contributor Author

@joedixon Yep. I tried it on a localhost setup Friday and the server started. Just tried it on my work's dev environment (that's the one that uses AWS with the Elasticache/Redis cluster) and it started up without any issues.

Thanks again for yours and Taylor's direction! I'm excited to dig into this more and finally move our app out of long polling and into websockets. 😁

@taylorotwell taylorotwell merged commit 78d8db3 into laravel:main Apr 22, 2024
9 checks passed
@MartyHimmel MartyHimmel deleted the redis-cluster branch April 22, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants