Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Lock] Fix StoreFactory to accept same DSN syntax as AbstractAdapter#36291
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jderusse 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.
I'm fine with this PR form a technical point of view.
But you have to keep in mind that using Lock with a cluster of Redis/memcached is not reliable: by defaultreplication is asynchronous, meaning, one process is able to acquire a Lock, which may not be propagated to replica, and, if primary has a failure, another process will be able to acquire the same lock .
You should use a CombineStore to achieve a HA with multiple Memcached/Redis
nb: This is already recommended in Documentation:https://symfony.com/doc/current/components/lock.html#id6
I think we should open a PR to add aWAIT in RedisStore that assure that a least 50%+1 servers in the cluster are synchronized
Uh oh!
There was an error while loading.Please reload this page.
jderusse commentedAug 6, 2020
fixed by#37590 |
fabpot commentedAug 6, 2020
Jontsa commentedAug 6, 2020
#37590 partially fixes this issue. StoreFactory still does not support the same exact DSN syntax as Symfony\Component\Cache\Adapter\AbstractAdapter for Redis connections. But this PR can be closed if we do not want to allow DSN with multiple Redis hosts for Locks. |
jderusse commentedAug 6, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You're right@Jontsa. Can you, please, rebase your PR and keep the part about DSN pattern? |
fabpot commentedSep 5, 2020
@Jontsa Do you time to finish this PR? |
fabpot commentedSep 11, 2020
@Jontsa I think you will need to rebase on the current 4.4 branch as there are some conflicts. |
jderusse left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
A small comment about change in tests.
Then, I'm 👍 with the PR
| yield ['memcached:?host[localhost]&host[localhost:12345]', MemcachedStore::class]; | ||
| } | ||
| if (class_exists(\Redis::class) &&class_exists(AbstractAdapter::class)) { | ||
| if ((class_exists(\Redis::class) ||class_exists(\Predis\Client::class)) &&class_exists(AbstractAdapter::class)) { |
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.
Revert this.
The purpose of this check is to avoid failing tests when the extension is not installed.
By checking if Predis is installed (which is always true because in require-dev) this breaks the initial check.
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.
fixed in6a79f3e
fabpot 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.
Taking over
fabpot commentedOct 3, 2020
Thank you@Jontsa. |
Updates
Symfony\Component\Lock\Store\StoreFactoryto accept same DSN syntax asSymfony\Component\Cache\Adapter\AbstractAdapterwhich is used to create Redis class instance.