Problem/Motivation

Maintaining the different implementations and adding both new backends (such as cluster) and new features (such as key value store) is complex and requires providing many classes with only slight variations.

Adding new connection options such as TLS or multiple servers is also challenging because of how the configuration is passed to the factory.

Steps to reproduce

Proposed resolution

Explore an adapter based approach. Identify unique cases that vary between implementations, also allow to pass through any command directly.

The adapter and the interface would need to be internal, while a goal would be to allow additional implementations, extending them must be an option to support new cases.

Find a better solution for the factory and its configuration. maybe a factory factory pattern with tagged services.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork redis-3080449

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Deprecate ClientInterface and ClientFactory::setClient() » Introduce Adapters instead of different cache/queue/lock classes per backend
Issue summary: View changes

berdir changed the visibility of the branch 3080449-deprecate-clientinterface-and to hidden.

berdir opened merge request !61

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Introduce Adapters instead of different cache/queue/lock classes per backend » Introduce client adapters instead of different cache/queue/lock classes per backend
Status: Active » Needs work

Started a proof of concept, all cache classes merged into one, getClient() actually returns an instanceof ClientInterface, which has some new methods for special things (really just multi/pipeline related things atm)

Currently using __call() to pass through all not explicitly defined methods directly to the actual client. Not sure yet if I want to rely on that (with @method on the interface maybe) or explicitly define all the methods used in our implementations and only keep that as a fallback.

Still need to update all the other things and refactor the factory part.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

One thing that I'm not sure about yet is the pipelines. I see two options:

* Add specific methods. That worked well for cache, it's just two and they are fairly generic. The reliable queue however has some very specific ones.
* Add our own generic version of that to the client adapter, with our own class to pass it through. the cluster version of that can then for example just not actually do a pipeline and pass it along as separate commands.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

This is now passing tests. I want to improve docs and so on, but it's seems to be working pretty well.

the multi stuff was easier than expected, I just can't use chained methods, then multi() seems to be working fine for both phpredis and predis. I might remove some or all of the methods I had to add and use non-chained multi() or pipeline calls.

acbramley’s picture

acbramley commented

Tested this and put together a cluster version. Using docker-compose and https://github.com/Grokzen/docker-redis-cluster

https://git.drupalcode.org/project/redis/-/merge_requests/62

Docker container:

  redis:
    image: grokzen/redis-cluster:latest
    network_mode: service:nginx
    environment:
      IP: '127.0.0.1'

Nginx ports:

      - '7000-7005:7000-7005'
      - '5000-5010:5000-5010'

It seems to be working well, info page is showing similar info to 1.9 + cluster patch.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The current pipeline approach might be a bit tedious for cluster, but I still think it should be possible. Alternatives would either be a lot of code for all (have a pipeline class for all implementations, force pipelines through that) or likely just as much complexity for predis (something like a function executePipeline(array $commands)).

I'm tempted to start with this approach, if it turns out too complicated for cluster I'm open to changing it.

  • berdir committed 3e7f5511 on 2.x 
    Issue #3080449 by berdir, acbramley: Introduce client adapters instead...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 8.x-1.x-dev » 2.x-dev
Status: Needs review » Fixed

Decided to go ahead and merge this into a new 2.x branch.

I want to stress that this is experimental and absolutely not ready for production and it will change and more breaking changes will be introduced. Started documenting the changes in a change record.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.