phpredis / phpredis

A PHP extension for Redis
Other
9.97k stars 2.14k forks source link

Support for RedisCluster pipelining? #1910

Open RafalLukawiecki opened 3 years ago

RafalLukawiecki commented 3 years ago

I see from the docs that the current implementation of RedisCluster does not support pipelining for reasons of data consistency. However, pipelining is useful for reasons of performance even under the risk of consistency, especially when using Redis as a cache.

Are there any plans for RedisCluster client to support pipelining (for multi)? Having this functionality would be of use to Drupal Redis module as it would enable it to directly support Redis Cluster mode without any major code modifications. There are currently 27000 web sites using Redis with that Drupal module, which, in turn, relies on phpredis or predis, and it seems that phpredis is more popular than predis based on the discussions about this module.

prattcmp commented 5 months ago

We can't use phpredis with Redis clustering in production because Laravel needs pipelining for job batch processing. It would be greatly appreciated if this capability could be added. 🙏🏽

michael-grunder commented 5 months ago

I'm not opposed to adding it in principle but we'd need to decide on various details.

  1. How to handle failure. We could be halfway through delivering a pipeline when something goes wrong, leaving half written payloads.
  2. Combining pipeline with multi, as can be done with standalone PhpRedis.
  3. What (if anything) we should do with special commands like MSET and MGET. Do we split the commands so the API is overall easier to use or just send what the user requests.

If we were to implement the functionality it might also be nice to do something clever on exec where we fcntl the underlying sockets to non-blocking and then read the replies from several different nodes concurrently.

prattcmp commented 5 months ago

I'm not an expert on Redis client implementations, but since pipelining is a client-side concept I think it would make sense to follow the behavior of other popular, stable clients as closely as possible.

1. For Failures: I think partial writes are fine. The PHP developer should understand that, while all pipeline requests will go out, not all are guaranteed to complete correctly or respond in the same order. In the case of Laravel, the batch (pipeline) feature does just that. It sends out multiple jobs at the same time, then lets the developer handle failures for any or all of those jobs either as they occur or after the entire pipeline is complete. It also gives the developer the option to kill the entire batch if any job fails. But again, that's left up to the developer, not done by the Redis client library.

2. Combining Pipeline with MULTI Each command in a MULTI block needs to be sent to the same shard. That seems to be the only issue.

3. Handling Special Commands like MSET and MGET I think it's reasonable to block MSET for pipelining for now. Laravel doesn't use MSET. For MGET, you could destructure them into subcommands that reside on the same node, similar to MULTI. You may even be able to use MULTI.

nolotz commented 4 hours ago

Hey @michael-grunder,

I'm currently running into some frustrating errors related to this, and I can't even see them anymore 😅. These issues are really driving me crazy as I've had to rewrite so many adapters.

Is there any update or plan to add support for pipelining in RedisCluster? It would be a game changer for our caching and job batch processing, even if there's some risk of inconsistency. Any guidance you could share would be appreciated!

Best regards, Noah