Skip to content

Conversation

@tompave
Copy link

@tompave tompave commented Jan 26, 2017

What

When a distributed Redis receives mget, find the nodes the keys should be
stored on and issue mget commands to each sub-node with only their matching keys.
Then combine the results and return them in an order that matches the key
list.

Why

My team is sharding the cache in a Rails application that uses Redis as cache storage.
The setup relies on:

redis-rails
  redis-activesupport
    redis-store
      redis

It works quite well.

However, we also use record-cache. That works too because it only uses the high level interface of the generic ActiveSupport cache. Unfortunately, it seems to be optimized to read records in batches, here:
https://github.com/orslumen/record-cache/blob/9bf4285c0a086f831b4b6e1d3fc292fbe44a14d6/lib/record_cache/multi_read.rb#L35-L43

The redis-activesupport cache adapter implementation of read_multi relies on mget, which currently raises an exception in Redis::Distributed.
This causes the cache queries to degenerate from a high number of mget to a very large number of single get commands.
The result is that we've observed a significant increase of both commands per second and CPU usage, which is what we actually wanted to reduce.

When a distributed redis receives mget, find the nodes the keys should be
stored on issue MGET commands to the sub-nodes with only their matching keys,
then combine the results and return them in an order that matches the key
list.
@badboy
Copy link
Contributor

badboy commented Jan 26, 2017

We are thinking about deprecating Redis::Distributed, hoping it to extract its functionality into an external gem.
Until we reach a discussion on that we won't merge additional code for it.

@badboy
Copy link
Contributor

badboy commented Jan 26, 2017

If you rely on Redis::Distributed this would be your chance to pick up maintainership of it though :)

@tompave
Copy link
Author

tompave commented Jan 26, 2017

I see. Yes, makes sense to hold off new additions then.

We rely on Redis quite heavily, and sharding it seems to be a good solution for us in the medium term.
I'd be happy to help, and thank you for the great work so far! 👍

@tompave
Copy link
Author

tompave commented Jan 26, 2017

Notes:

CI failed on MRI 1.8.7 and JRuby in 1.8 mode because of the lack of Enumerable#flat_map.
Another Failure on JRuby in 1.9 mode, but it looks unrelated.

@badboy
Copy link
Contributor

badboy commented Jan 26, 2017

MRI 1.8.7 and JRuby <9 will likely be dropped with the next release as well

@djanowski
Copy link
Collaborator

Implemented by #687. Thank you very much.

@djanowski djanowski closed this Aug 25, 2017
@tompave
Copy link
Author

tompave commented Aug 26, 2017

You're welcome @djanowski. It's a shame that a later PR from @jeremy with the same approach was preferred over this, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants