-
Notifications
You must be signed in to change notification settings - Fork 47
Implementing PhpRedis client #36
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
Conversation
7897b9e to
028b794
Compare
83b0947 to
11a7bc9
Compare
11a7bc9 to
dcd0370
Compare
@Namoshek
Namoshek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this looks very promising and exactly what I need for an upcoming move to Kubernetes. Thanks for putting so much effort into making this a thing! Hopefully, the maintainer is going to be able to have a look at this soon.
@Namoshek
Namoshek
Apr 18, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these configuration paramters play together with retry_limit and retry_wait? Would it be possible to allow the PhpRedisConnection to reuse the connector logic (and retry configuration) of PhpRedisConnector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings allow for a different configuration on first connection. So the normal retry_limit and retry_wait are used for reconnecting after a failure. The connector_ options are used when creating the connection for the first time.
I've combined some of the retry logic, but I can't seem to find a way to simplify this. Maybe you have some ideas about it?
@Namoshek
Namoshek
May 14, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the settings are fine, just a bit hard to understand in combination with the other ones (which have quite similar names too). Explaining their difference from the similar named settings is probably all you can do at this point.
@Namoshek thanks for your reply, I will try to work on your comments next week. We are already using this PR in our Kubernetes cluster :)
krowinski
commented
Oct 15, 2021
Hi! Any ETA when this will be merge to main tree and tag released ?
L3o-pold
commented
May 2, 2022
@cyrossignol can we see this merged
@L3o-pold
L3o-pold
Aug 10, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should extract the options key before setting the servers or it can result trying to connect to the wrong hostname here: https://github.com/monospice/laravel-redis-sentinel-drivers/pull/36/files#diff-3bf85f59585710cebd9ae7787d8b7ffb7df273ce8ffe2e72053ef96b4bb81ccaR157
@L3o-pold
L3o-pold
Aug 10, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are using $options instead of $clientOpts
Uh oh!
There was an error while loading. Please reload this page.
This PR implements the PhpRedis client for Redis Sentinel. It wraps all methods with a retryOnFailure and creates a new Redis client when the connection fails.