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

allow sentinel replication configuration #307

Merged
merged 3 commits into from
Mar 15, 2017
Merged

allow sentinel replication configuration #307

merged 3 commits into from
Mar 15, 2017

Conversation

othillo
Copy link
Contributor

@othillo othillo commented Dec 5, 2016

I would like to use this bundle with a sentinel setup. This pull request allows setting the replication option to 'sentinel' and allows setting the 'service' option.

related to #274

@othillo othillo changed the title allow sentinel replication configuration [WIP] allow sentinel replication configuration Dec 5, 2016
@othillo othillo changed the title [WIP] allow sentinel replication configuration allow sentinel replication configuration Dec 5, 2016
@MyDigitalLife MyDigitalLife mentioned this pull request Jan 17, 2017
@MyDigitalLife
Copy link

I checked out this code and it works as i was hopping in my test environment. I did however add a extra parameter to make sure selecting a database is an option.

@@ -135,7 +135,8 @@ private function addClientsSection(ArrayNodeDefinition $rootNode)
->end()
->scalarNode('cluster')->defaultNull()->end()
->scalarNode('prefix')->defaultNull()->end()
->booleanNode('replication')->defaultFalse()->end()
->enumNode('replication')->values(array(true, false, 'sentinel'))->defaultNull()->end()
->scalarNode('service')->defaultNull()->end()

Choose a reason for hiding this comment

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

Adding this will enable people to select a database if they choose:
->arrayNode('parameters') ->children() ->scalarNode('database')->defaultNull()->end() ->end() ->end()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MyDigitalLife do you mind creating a pull request for this with a test case?

@othillo
Copy link
Contributor Author

othillo commented Jan 25, 2017

I removed the defaultNull for the replication option as suggested by @ivoba in #305

Also added a test for the cluster configuration where the replication option must not be set.

@snc
Copy link
Owner

snc commented Jan 31, 2017

What does service: mymaster is for? I think it would be nice to add something to the docs about sentinal support.

@MyDigitalLife
Copy link

MyDigitalLife commented Jan 31, 2017

@snc service: mymaster is needed to specify a set of instances. This is done within the sentinel config file. Its basicly the name of your sentinel cluster. You can find some more information about this under "Configuring Sentinel" here: https://redis.io/topics/sentinel

@MyDigitalLife
Copy link

Is there a time table when this feature will be merged?

@ibrambe
Copy link

ibrambe commented Mar 2, 2017

I'm also interested in this feature.

@ricbra
Copy link

ricbra commented Mar 10, 2017

Any update on why this isn't merged yet?

@snc
Copy link
Owner

snc commented Mar 10, 2017

@othillo Could you add some lines to the readme about this feature?

@othillo
Copy link
Contributor Author

othillo commented Mar 11, 2017

@snc done :) b61f0f5

service: mymaster
```

The `service` is the name of the set or Redis instances.
Copy link
Owner

Choose a reason for hiding this comment

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

Should be "of Redis instances" right? :-)

@snc snc merged commit 2c6b4ac into snc:master Mar 15, 2017
@snc
Copy link
Owner

snc commented Mar 15, 2017

Thanks, merged.

@othillo
Copy link
Contributor Author

othillo commented Mar 15, 2017

thanks! would you mind tagging a new release?

type: predis
alias: default
dsn:
- redis://localhost

Choose a reason for hiding this comment

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

Are these sentinel hosts or redis hosts? What about port? Does the replication: sentinel setting automatically use port 26379 on those hostnames?

@ricbra
Copy link

ricbra commented Apr 21, 2017

Could you tag a new release? @snc

@mnapier82
Copy link

For anyone subscribed to this I see that a new release was tagged 8 days ago that includes this.
Thank you!

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.

7 participants