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

removed defaultValue for replication option to allow cluster option #305

Closed
wants to merge 1 commit into from

Conversation

ivoba
Copy link

@ivoba ivoba commented Dec 1, 2016

Predis now has built-in support for redis-cluster:
https://github.com/nrk/predis/blob/v1.1/README.md#cluster

However this options has only effect when the replication option is omitted.
But due to the defaultFalse configuration, the option['replication'] was always present and prevented Predis to enter cluster mode:
https://github.com/nrk/predis/blob/v1.1/src/Client.php#L123

An example config is:

snc_redis:
    clients:
      default:
          type: predis
          alias: default
          dsn:
              - 'redis://%cache.redis_host.one%:%cache.redis_port%/%cache.redis_db%'
              - 'redis://%cache.redis_host.two%:%cache.redis_port%/%cache.redis_db%'
          options:
            profile: 3.2
            cluster: "redis"

With "redis-custer" or "redis" as cluster option commands like this will automatically be resolved:
MOVED 14966 cache.redis_host.two:6381

This probably needs some more work as replication now can be even more than boolean:
https://github.com/nrk/predis/blob/v1.1/README.md#replication

@snc
Copy link
Owner

snc commented Dec 11, 2016

Does your setup work with #307?

@ivoba
Copy link
Author

ivoba commented Dec 13, 2016

@snc #307 will not work for cluster: "redis" because the options array will still have the key "replication" with null and so the isset function will not let the option pass.
When ->defaultNull() would be removed it would probably work.

However as i mentioned in #306 i would prefer not to predefine/limit the options by the bundle configuration at all.

@jontorrado
Copy link

What about merging this PR @snc? This fixes redis cluster for newer predis versions.

@ebarault
Copy link

ebarault commented Mar 22, 2018

@ivoba @snc is there any progress on this?

Is there currently a working config with predis 1.1.1 that allows cluster: redis option to work ?

I'm using AWS Elasticache Redis with configuration endpoint and can't find a way to solve MOVED 14966 ... exceptions

@curry684 curry684 added this to the 3.0 milestone Apr 6, 2018
@curry684
Copy link
Collaborator

curry684 commented Apr 7, 2018

I'm closing this PR as the line of code it's changing has already been modified since, and no longer has a default anyway in that new implementation so it's no longer relevant.

@curry684 curry684 closed this Apr 7, 2018
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.

5 participants