monospice / laravel-redis-sentinel-drivers

Redis Sentinel integration for Laravel and Lumen.
MIT License
101 stars 48 forks source link

Multiple host configuration not as readme currently states #3

Closed jacovdbergh closed 7 years ago

jacovdbergh commented 7 years ago

https://github.com/monospice/laravel-redis-sentinel-drivers#multi-service-configuration

Using that config array did not work for me, the following did work:

redis-sentinel => [
     "queue" => [
       "10.0.0.1:26379",
       "10.0.0.2:26379",
       "options" => [
         "service" => "db1",
         "parameters" => [
           "database" => 1,
           "password" => null,
         ],
       ],
     ],
     "cache" => [
       "10.0.0.1:26379",
       "10.0.0.2:26379",
       "options" => [
         "service" => "db1",
         "parameters" => [
           "database" => 2,
           "password" => null,
         ],
       ],
     ],
     "session" => [
       "10.0.0.1:26379",
       "10.0.0.2:26379",
       "options" => [
         "service" => "db1",
         "parameters" => [
           "database" => 3,
           "password" => null,
         ],
       ],
     ],
   ]

Is it something I'm doing wrong? Using Predis 1.1, Laravel 5.2

cyrossignol commented 7 years ago

The code in your post may have a couple typos: looks like it's using invalid IP addresses, 10.0.01 and 10.0.02, and you might need to put the redis-sentinel array key in quotes.

Predis supports multiple formats for declaring Redis hosts, such as the ip.address:port variation you used above, or the format described in the README which eases the use of environment variables. I'm fairly certain that format works because we've been using it in production.

Could you please describe in more detail what does not work? I can offer better advice if you provide the symptoms or errors you experienced.

jacovdbergh commented 7 years ago

Thanks for the reply.

The code in my post is actually the snippet that finally worked for me, I introduced a few errors copying the code here. I changed the IPs to something more easily readable than our internal server IPs, should have been 10.0.0.1 and 10.0.0.2, and just forgot the quote around 'redis-sentinel'.

But that aside, I ran into another issue after getting it to work.

The session driver seems to work fine, did not test the Cache, but I get the following error when trying to use the queue driver: Cannot initialize a MULTI/EXEC transaction over aggregate connections. This happens when Illuminate\Queue\RedisQueue::migrateAllExpiredJobs executes.

I'm using Laravel 5.2. Any ideas?

cyrossignol commented 7 years ago

Ah, good catch. The Laravel queue implementation executes the related redis commands as a transaction, but the Predis client doesn't support transactions over aggregate connections like those used for Sentinel. Please create a separate ticket for this. I'll start looking into a solution.

I'm glad you resolved your other issue by changing the format of the config array, but I'm still curious to know why the original format didn't work for you. If you can, please let me know what symptoms or errors you experienced so we can fix the bug if one exists. An excerpt of your original, non-working code might be helpful as well. Thanks!

jacovdbergh commented 7 years ago

I see in 5.3 the migrateExpiredJobs uses LuaScripts instead of a transaction, which might fix the issue I have, but I'll create a ticket for <5.2.

I tried the following for config\database.php and some variations, all of which resulted in the default parameters for host and port being used:

 'redis-sentinel' => [

        'default' => [
            [
                'host' => '10.200.101.136',
                'port' => '26379',
            ],
        ],

        'options' => [
            'service' => 'db1',
            'parameters' => [
                'password' => env('REDIS_PASSWORD', null),
                'database' => 0,
            ],
        ],

    ],

The default I kept getting being

private static $defaults = array(
        'scheme' => 'tcp',
        'host' => '127.0.0.1',
        'port' => 6379,
    );

from predis\src\Connection\Parameters.php.

cyrossignol commented 7 years ago

@jacovdbergh, how did you check which parameters the client ended up with for your example? I tried several variations to reproduce this behavior, but I can't seem to get it to fallback to the defaults in the Predis package.

Using the configuration in your post above, the package seems to be working with L52 and Predis 1.1:

dd(app('redis-sentinel')->connection()->getConnection()->getSentinelConnection());

Predis\Connection\StreamConnection {#637
  -resource: null
  -cachedId: null
  #parameters: Predis\Connection\Parameters {#638
    -parameters: array:6 [
      "host" => "10.200.101.136"
      "port" => "26379"
      "database" => null
      "password" => null
      "timeout" => 0.1
      "scheme" => "tcp"
    ]
  }
  #initCommands: []
}

For me, the client is initialized with the correct hosts. Let me know if you can think of any more information to help diagnose this.

jacovdbergh commented 7 years ago

@cyrossignol sorry for the delay, was away for a bit.

I'll have a look and get back to you on this, might just as well be something stupid I did on my end.

Thanks for the updates for 5.4 as well, gonna start testing with them this week.

jacovdbergh commented 7 years ago

In conclusion I'm pretty sure it was something stupid on my end.