[Lock] Fix StoreFactory to accept same DSN syntax as AbstractAdapter #36291
Conversation
Jontsa
changed the title
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 default replication 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 a WAIT in RedisStore that assure that a least 50%+1 servers in the cluster are synchronized
|
fixed by #37590 |
|
#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. |
|
You're right @Jontsa. Can you, please, rebase your PR and keep the part about DSN pattern? |
|
@Jontsa Do you time to finish this PR? |
|
@Jontsa I think you will need to rebase on the current 4.4 branch as there are some conflicts. |
| } | ||
| 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.
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.
|
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.