Create cluster connection correctly for one host #416

linasm83 · 2018-04-18T21:51:19Z

Fixes #402, #267 (might also fix some other clustering issues when only one dsn is defined)

If only one dsn is defined when cluster option is passed connection instance class is incorrect (should be Predis\Connection\Aggregate\RedisCluster.

The problem is that we're passing Predis\Connection\ParametersInterface as first argument to client, but we should pass array instead cause only then Predis\Client::createConnection method will create correct connection class.

curry684 · 2018-04-19T08:59:58Z

Seems simple enough, since I don't have clusters to test on hoping we can get confirmation of the fix from people that experienced issues here!

curry684 · 2018-04-23T12:59:17Z

@linasm83 do you understand the issues reported at #402 (comment)?

linasm83 · 2018-04-23T21:54:52Z

@curry684 I think it should only work with replication: false if setting replication: true then you need to provide at least two dsn's for predis client (one master / multiple slaves).

Looking at predis client class it looks it only supports cluster or replication, but not both at the same time.

So not sure there's proper solution on that case, maybe only some additional config validation not allow to pass only one dsn if replication: true?

curry684 · 2018-04-24T11:05:52Z

Makes sense that you cannot use both at the same time, would cause awful race conditions I suppose. In that case yes we should fail early during compilation.

@ebarault what's your take on this?

ebarault · 2018-04-24T14:32:37Z

The purpose of backend-side-managed clustering is to get rid of any extra configuration at client side.
Hence we could completely drop the requirement for the replication param when cluster type redis is selected.

This config should fail with an error early and with the appropriate error message.

ignaciorivmen · 2018-04-25T18:24:11Z

Hi,

I have tested this modification on my local machine and on AWS but I'm getting the next error :

[Symfony\Component\Debug\Exception\ContextErrorException]
Warning: call_user_func_array() expects parameter 1 to be a valid callback, no array or string given

Exception trace:
() at /home/irivas/workspace/hopmedia/valorise/vendor/predis/predis/src/Client.php:162
Symfony\Component\Debug\ErrorHandler->handleError() at n/a:n/a
call_user_func_array() at /home/irivas/workspace/hopmedia/valorise/vendor/predis/predis/src/Client.php:162
Predis\Client->Predis{closure}() at /home/irivas/workspace/hopmedia/valorise/vendor/predis/predis/src/Client.php:131
Predis\Client->createConnection() at /home/irivas/workspace/hopmedia/valorise/vendor/predis/predis/src/Client.php:56
Predis\Client->__construct() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:7385
appDevDebugProjectContainer->getSncRedis_DefaultService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:7405
appDevDebugProjectContainer->getSncRedis_Session_HandlerService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:7305
appDevDebugProjectContainer->getSession_Storage_NativeService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:7265
appDevDebugProjectContainer->getSessionService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:6929
appDevDebugProjectContainer->getSecurity_Csrf_TokenManagerService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:7977
appDevDebugProjectContainer->getTwigService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:2691
appDevDebugProjectContainer->getBazinga_Jstranslation_TranslationDumperService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/app/cache/dev/appDevDebugProjectContainer.php:2679
appDevDebugProjectContainer->getBazinga_Jstranslation_DumpCommandService() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:297
Symfony\Component\DependencyInjection\Container->get() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:147
Symfony\Bundle\FrameworkBundle\Console\Application->registerCommands() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:112
Symfony\Bundle\FrameworkBundle\Console\Application->all() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:66
Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /home/irivas/workspace/hopmedia/valorise/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:117
Symfony\Component\Console\Application->run() at /home/irivas/workspace/hopmedia/valorise/app/console:29

My configuration :

snc_redis:
clients:
default:
type: predis
alias: default
dsn:
- %redis_dsn_1%
options:
profile: 3.2
cluster: redis
replication: false

session:
client: default
prefix: "%session_prefix%"

ebarault · 2018-04-25T18:29:07Z

this is with predis version 1.1.1 (latest released)

linasm83 · 2018-04-25T19:10:24Z

@ignaciorivmen try to remove replication: false at all

snc_redis:
    clients:
        default:
           type: predis
           alias: default
           dsn:
               - %redis_dsn_1%
           options:
               profile: 3.2
               cluster: redis
    session:
        client: default
        prefix: "%session_prefix%"

maybe we should really ignore replication parameter when cluster: redis is defined

ebarault · 2018-04-25T19:45:05Z

bingo: we tried without the replication param and now the app compiles

the app seems to behave normally, but we get these errors in symfony logs:

snc_redis.ERROR: Command "SET session_prefix_dev135c5b319c119c6e6ce8da0524a8ab9d.lock 5ae0d948658e9 PX 30001 NX" failed (MOVED 7537 172.50.197.202:6379)

in other words, the storage of sessions in redis seems to fail

EDIT: after checking in the redis cluster: the key session_prefix_dev135c5b319c119c6e6ce8da0524a8ab9d is stored in the 2 nodes of the cluster's single shard

Is is possible that this error is in fact just a warning before the client redirects to the right endpoint?

linasm83 · 2018-04-25T19:55:05Z

EDIT: Might be unexpected behavior of connection wrapper class in such case.
Personally not getting this error, but might due some other configuration difference like for example for session writing we use SETEX instead of SET cause we provide ttl

curry684 · 2018-04-26T12:13:37Z

Meh the wrappers are causing a lot of issues considering they were written ages before PHP started supporting type safety in function arguments.

ebarault · 2018-04-26T12:46:08Z

I'm not sure which additional test could be ran to bring more food for thoughts.
From a black-box perspective, i'd say the fix works.

curry684 · 2018-05-07T21:58:37Z

maybe we should really ignore replication parameter when cluster: redis is defined

Sounds like a good idea if that causes the fix to be universally applicable, could you add that?

linasm83 · 2018-05-08T21:11:25Z

@curry684 Added.

curry684 · 2018-05-09T08:08:47Z

Thanks!

* Create cluster connection correctly * Unset replication option if cluster option is defined for predis client (cherry picked from commit 5d8169e)

ebarault · 2018-05-16T09:58:19Z

@curry684 : hi, could you tag a new release and propagate this fix to composer ?

curry684 · 2018-05-16T11:29:24Z

Yes, been running 2.1.x for a few days myself now in current project and it seems all stable.

curry684 · 2018-05-16T11:32:27Z

https://github.com/snc/SncRedisBundle/releases/tag/2.1.3

ebarault · 2018-05-16T13:27:54Z

thanks 👍

Create cluster connection correctly

e93c617

curry684 mentioned this pull request Apr 19, 2018

AWS Elasticache Redis Cluster : Uncaught PHP Exception Predis\Response\ServerException: MOVED #402

Closed

Unset replication option if cluster option is defined for predis client

8d63ac5

curry684 merged commit 5d8169e into snc:2.1 May 9, 2018

curry684 mentioned this pull request May 9, 2018

DSN defination problem: Cluster hostname #267

Closed

curry684 pushed a commit that referenced this pull request May 9, 2018

Create cluster connection correctly for one host (#416)

38de7ed

* Create cluster connection correctly * Unset replication option if cluster option is defined for predis client (cherry picked from commit 5d8169e)

linasm83 deleted the cluster_connection_fix branch May 10, 2018 05:49

ebarault mentioned this pull request Oct 19, 2018

falsy "failed (MOVED)" error logs when using cluster mode (following #416) #466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create cluster connection correctly for one host #416

Create cluster connection correctly for one host #416

linasm83 commented Apr 18, 2018

curry684 commented Apr 19, 2018

curry684 commented Apr 23, 2018

linasm83 commented Apr 23, 2018 •

edited

Loading

curry684 commented Apr 24, 2018

ebarault commented Apr 24, 2018

ignaciorivmen commented Apr 25, 2018

ebarault commented Apr 25, 2018 •

edited

Loading

linasm83 commented Apr 25, 2018

ebarault commented Apr 25, 2018 •

edited

Loading

linasm83 commented Apr 25, 2018 •

edited

Loading

curry684 commented Apr 26, 2018

ebarault commented Apr 26, 2018

curry684 commented May 7, 2018

linasm83 commented May 8, 2018

curry684 commented May 9, 2018

ebarault commented May 16, 2018

curry684 commented May 16, 2018

curry684 commented May 16, 2018

ebarault commented May 16, 2018

Create cluster connection correctly for one host #416

Create cluster connection correctly for one host #416

Conversation

linasm83 commented Apr 18, 2018

curry684 commented Apr 19, 2018

curry684 commented Apr 23, 2018

linasm83 commented Apr 23, 2018 • edited Loading

curry684 commented Apr 24, 2018

ebarault commented Apr 24, 2018

ignaciorivmen commented Apr 25, 2018

ebarault commented Apr 25, 2018 • edited Loading

linasm83 commented Apr 25, 2018

ebarault commented Apr 25, 2018 • edited Loading

linasm83 commented Apr 25, 2018 • edited Loading

curry684 commented Apr 26, 2018

ebarault commented Apr 26, 2018

curry684 commented May 7, 2018

linasm83 commented May 8, 2018

curry684 commented May 9, 2018

ebarault commented May 16, 2018

curry684 commented May 16, 2018

curry684 commented May 16, 2018

ebarault commented May 16, 2018

linasm83 commented Apr 23, 2018 •

edited

Loading

ebarault commented Apr 25, 2018 •

edited

Loading

ebarault commented Apr 25, 2018 •

edited

Loading

linasm83 commented Apr 25, 2018 •

edited

Loading