dskit icon indicating copy to clipboard operation
dskit copied to clipboard

Redis cluster support

Open dimitarvdimitrov opened this issue 2 years ago • 3 comments
trafficstars

Initially reported in a grafana/mimir issue https://github.com/grafana/mimir/issues/5896

When using mget we need to provide keys that hash to the same slot. This is not a consideration that the existing client makes, so the larger the redis cluster is, the more likely it is that the multiple keys hash to different slots and the less likely it is that multi-key GETs work.

Redis keyslot distribution is documented in Key distribution model in this doc

Proposals

Some proposals to start the discussion

  • Do single key lookups; this has the obvious impact of making lookups slower
  • Using CRC16 find the cluster slots of each key. Then issue mget commands each unique slot and the keys that hash to it.

dimitarvdimitrov avatar Sep 04 '23 14:09 dimitarvdimitrov

Redis support is experimental and unsupported. IMO it shouldn't have ever been added to dskit or Mimir. We don't run it and have no intention of running it. I propose not fixing this issue or spending anytime on Redis unless there is a customer requirement for it.

56quarters avatar Sep 05 '23 13:09 56quarters

Redis support is experimental and unsupported. IMO it shouldn't have ever been added to dskit or Mimir. We don't run it and have no intention of running it. I propose not fixing this issue or spending anytime on Redis unless there is a customer requirement for it.

This comment seems unnecessarily hostile. Redis is a very widely used caching solution, just because you don't run it doesn't mean no one does. In our own case, we already have Redis installed. If we asked to install Memcache, the response would be "Why do you need another cache when we already have Redis?"

Furthermore, Redis support is also in both Tempo and Loki, and it's not even marked as experimental in Loki. My understanding of dskit is that it's intended to be a central library that consolidates the implementations of utilities into a single dependency, so that all of Grafana's components use the same implementation instead of their own separate implementation. If that is the case, the Redis Client easily matches that description.

zackman0010 avatar Sep 05 '23 14:09 zackman0010

Redis support is experimental and unsupported. IMO it shouldn't have ever been added to dskit or Mimir. We don't run it and have no intention of running it. I propose not fixing this issue or spending anytime on Redis unless there is a customer requirement for it.

This comment seems unnecessarily hostile. Redis is a very widely used caching solution, just because you don't run it doesn't mean no one does. In our own case, we already have Redis installed. If we asked to install Memcache, the response would be "Why do you need another cache when we already have Redis?"

That's the exact same logic I'm applying here. We don't run it internally, have no reason to start, and hence can't support it.

Furthermore, Redis support is also in both Tempo and Loki, and it's not even marked as experimental in Loki. My understanding of dskit is that it's intended to be a central library that consolidates the implementations of utilities into a single dependency, so that all of Grafana's components use the same implementation instead of their own separate implementation. If that is the case, the Redis Client easily matches that description.

Tempo and Loki are their own projects and can make their own decisions about what they want to support.

56quarters avatar Sep 05 '23 15:09 56quarters