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

[8.x] Adding support for using a different Redis DB in a Sentinel setup #38764

Merged

Conversation

tim-hanssen
Copy link
Contributor

@tim-hanssen tim-hanssen commented Sep 11, 2021

Following the Cache and Database documentation, it should be possible to create multiple cache stores from the Laravel database config.

This doest work if someone is using a Redis Sentinel configuration.

The host can be changed, but not the selected DB. This change won't break current configurations.

Current config:

    'redis' => [

        'client' => env('REDIS_CLIENT', 'phpredis'),

        'options' => [
            'replication' => 'sentinel',
            'service' => 'mymaster',
            'parameters'  => [
                    'password' => env('REDIS_PASSWORD', null),
                    'database' => env('REDIS_DATABASE', null),
            ],
        ],

        'default' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
        ],

        'storetwo' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
        ],
    ],

New config:

    'redis' => [

        'client' => env('REDIS_CLIENT', 'phpredis'),

        'options' => [
            'replication' => 'sentinel',
            'service' => 'mymaster',
            'parameters'  => [
                'default' => [
                    'password' => env('REDIS_PASSWORD', null),
                    'database' => env('REDIS_DATABASE', null),
                ],
                'storetwo' => [
                    'password' => env('REDIS_PASSWORD', null),
                    'database' => env('REDIS_DATABASE_TWO', null)
                ]
            ],
        ],

        'default' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
        ],

        'storetwo' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
        ],
    ],

@GrahamCampbell GrahamCampbell changed the title Adding support for using a different Redis DB in a Sentinel setup [8.x] Adding support for using a different Redis DB in a Sentinel setup Sep 11, 2021
@GrahamCampbell
Copy link
Member

Please can you include some tests?

@tim-hanssen
Copy link
Contributor Author

@GrahamCampbell sure, I added a connection test.

@driesvints
Copy link
Member

Wouldn't it be better to override options directly on the stores instead of in the options array?

@tim-hanssen
Copy link
Contributor Author

If you would ask me @driesvints I think this would make more sense;

` 'redis' => [

    'client' => env('REDIS_CLIENT', 'phpredis'),
    
    'default' => [
        'hosts' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
            ],
        'options' => [
            'replication' => 'sentinel',
            'service' => 'mymaster',
            'parameters'  => [
                    'password' => env('REDIS_PASSWORD', null),
                    'database' => env('REDIS_DATABASE', null)
            ],
        ]
    ],

    'storetwo' => [
        'hosts' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
            ],
        'options' => [
            'replication' => 'sentinel',
            'service' => 'mymaster',
            'parameters'  => [
                    'password' => env('REDIS_PASSWORD', null),
                    'database' => env('REDIS_DATABASE_TWO', null)
            ],
        ]
    ],
],`

@driesvints
Copy link
Member

That's a big breaking change so we cannot do that. Maybe something more like:

    'redis' => [

        'client' => env('REDIS_CLIENT', 'phpredis'),

        'options' => [
            'replication' => 'sentinel',
            'service' => 'mymaster',
        ],

        'default' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
            'options' => [
                'parameters'  => [
                        'password' => env('REDIS_PASSWORD', null),
                        'database' => env('REDIS_DATABASE', null),
                ],
            ],
        ],

        'storetwo' => [
            'tcp://'.env('REDIS_HOST_1').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_2').':26379?timeout=0.100',
            'tcp://'.env('REDIS_HOST_3').':26379?timeout=0.100',
            'options' => [
                'parameters'  => [
                        'password' => env('REDIS_PASSWORD', null),
                        'database' => env('REDIS_DATABASE_TWO', null),
                ],
            ],
        ],
    ],

But not ideal. I agree that your solution is better but every app would break if we changed it like that.

@tim-hanssen
Copy link
Contributor Author

tim-hanssen commented Sep 13, 2021

Yes I agree, that last comment looks to me as the most easy way, would you like me to reflect that into the commit @driesvints?

@driesvints
Copy link
Member

Let's see what Taylor says.

@taylorotwell
Copy link
Member

Where do we even document that you can pass these replication option keys, etc.?

@tim-hanssen
Copy link
Contributor Author

I don't think there are any docs on how to do this, but since the config is passed to predis, it's just works with their documentation (except the store parameters). But Sentinel is a very much used setup in a more enterprise infra.

@taylorotwell taylorotwell merged commit fa87704 into laravel:8.x Sep 13, 2021
wouterj pushed a commit to wouterj/laravel-framework that referenced this pull request Sep 15, 2021
…up (laravel#38764)

* Update RedisManager.php

* Update RedisManager.php

* Add test for Sentinel connections
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
…up (laravel#38764)

* Update RedisManager.php

* Update RedisManager.php

* Add test for Sentinel connections
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.

4 participants