Add support for persistent connections to redis host

The patch failed to apply on module version 8.x-1.0.

patch "-p1" --no-backup-if-mismatch -d "modules/contrib/redis" < "C:\Users\kdoyle\AppData\Local\Temp/5b7319dcc1449.patch"
patching file src/Client/PhpRedis.php
Hunk #1 FAILED at 19.
1 out of 1 hunk FAILED -- saving rejects to file src/Client/PhpRedis.php.rej

patch "-p0" --no-backup-if-mismatch -d "modules/contrib/redis" < "C:\Users\kdoyle\AppData\Local\Temp/5b7319dcc1449.patch"
can't find file to patch at input line 14

 class Predis implements ClientInterface {
 
-  public function getClient($host = NULL, $port = NULL, $base = NULL, $password = NULL, $replicationHosts = NULL) {
+  public function getClient($host = NULL, $port = NULL, $base = NULL, $password = NULL, $persistent = FALSE, $replicationHosts = NULL) {
     $connectionInfo = [

I guess the order of parameters (even if they are optional) should not change within the same major version. So this change either requires a major version bump or creating dedicated classes for persistent connections, for example: PredisPersistent and PhpRedisPersistent. The ClientFactory could initiate the correct class based on the settings.

Agreed, new arguments should be at the end.

Changed ordering of arguments as per @mxr576 's comment

Tested and made sure that the connection remains persistent and no new connections are opened on new requests with redis stats.

Thanks, committed.

Had to revert this. This causes notices when not using replication hosts as the order of arguments there is then wrong. See https://travis-ci.org/md-systems/redis/jobs/583318293

I also see now why the argument was added before, because existing ones don't exist there.

I'm honestly unhappy about the whole client stuff, and how that's currently handled. Was kinda parted from 7.x but that moved on to a different architecture as well.

An idea for a new 8.x-2.x might be to have an adapter class that passes through the commands to each API, then we wouldn't have to duplicate all the implementations.

For now, lets just fix that notice, ignore the warnings about wrong parameters on the interface and I'll create a follow-up to deprecate ClientInterface. It's basically unused.

@Berdir,

I am not sure. Will exchanging parameters at line 14 of src/Client/Predis.php will work?

+++ b/src/ClientFactory.php
@@ -165,14 +166,16 @@ class ClientFactory {
           $settings['port'],
           $settings['base'],
-          $settings['password']);
+          $settings['password'],
+          $settings['persistent']);
       }

No, the call to it needs to be updated here, you need to pass an empty array for the replication hosts.

Patch, tested as per @Berdir's comment

https://github.com/md-systems/redis/pull/30

Hey guys, I was testing this patch in our infrastructure using phpredis and couldn't make it work.

After some digging I saw that instad of the persistent flag the function was getting an empty array. It looks like the function getClient is missing an argument
instead of:

public function getClient($host = NULL, $port = NULL, $base = NULL, $password = NULL, $persistent = FALSE) {

should be:

public function getClient($host = NULL, $port = NULL, $base = NULL, $password = NULL, $replicationHosts = NULL, $persistent = FALSE) {

Here is a reroll of #17. No interdiff (of course). Will look into the open feedback next.

An idea for a new 8.x-2.x might be to have an adapter class that passes through the commands to each API, then we wouldn't have to duplicate all the implementations.

+1 for this. I am trying to sort out the situation with that getClient method and it is a... well, I'll stop there or I'll say something nasty. Suffice to say, it does *not* scale when we keep adding new configuration options that need to be passed to the clients. Also, I guess a NTH for any new architecture would be an early warning mechanism that can give a warning when a client does not support a config. Like the replication hosts is imply not touched by the PhpRedis client adapter.

Edit: I'd say what would make most sense is if the backend setting would actually determine what factory class to load. The factory class can then read any settings it might like and produce a client based on it. It would still have a getClient() method, but without all the parameters (actually, that would mean my warning system wouldn't get used, unsupported settings would get ignored, like they are now).

This adds the replication hosts to the interface and to the PhpRedis implementation (where it really has no business... :)).

I've applied patches #17, #20 and #22.

Debugging locally, I can see that #17 does not work since $persistent is always an empty array. #20 and #22 do call pconnect instead of connect.

However, analyzing the New connections chart on AWS RDS, I was expecting changes in the numbers after applying the patches. I'm not seeing that though.

We have some cron jobs responsible to query specific nodes and create some group content (entities). This is the chart analysis.

Can someone explain if this is expected? If yes, what's the main goal of applying the pconnect patch?

Guys,

After some investigation, we find out that the spikes and numbers above were actually being caused by PHP workers that we have running on the environments.

I tested locally and the connections were persistent. Also tested on the environments disabling the workers and noticed a huge improvement when performing Bulk updates for example.

We have just released the patch and I'm attaching the evidence that it's working for us.

Thank you :)

Thanks, committed.