-
Notifications
You must be signed in to change notification settings - Fork 21.8k
Built-in Redis cache store #31134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Built-in Redis cache store #31134
Conversation
915bc90
to
b1a47ad
Compare
.travis.yml
Outdated
@@ -41,7 +42,7 @@ before_script: | |||
# Decodes to e.g. `export VARIABLE=VALUE` | |||
- $(base64 --decode <<< "ZXhwb3J0IFNBVUNFX0FDQ0VTU19LRVk9YTAzNTM0M2YtZTkyMi00MGIzLWFhM2MtMDZiM2VhNjM1YzQ4") | |||
- $(base64 --decode <<< "ZXhwb3J0IFNBVUNFX1VTRVJOQU1FPXJ1YnlvbnJhaWxz") | |||
- redis-server --bind 127.0.0.1 --port 6379 --requirepass 'password' --daemonize yes | |||
- redis-server --bind 127.0.0.1 --port 6380 --requirepass 'password' --daemonize yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate Redis for Action Cable tests which specifically want to work against a non-default config.
Use the default redis-server
service for Active Support cache store tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have the test suite spin this up & down directly? Having to set a password on my local redis to get the tests to pass was annoying, but making everyone manually run a second instance seems even worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I'd rather change the Action Cable tests separately. The point of this was to demonstrate that AC would accept user credentials for Redis. A mocked test should be sufficient. Then we can use a default Redis config for all tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #31136
"#{namespace}:#{key}" | ||
else | ||
key | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated this method from normalize_key
so they may be overridden and called independently by subclasses.
The Redis cache store hashes cache keys larger than 1kB and it namespaces deleted_matched
globs, which aren't keys to be normalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was removed from public API in Rails 5.0. #22215 for context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll call it namespace_key
instead, matching verb tense with normalize_key
. (It's a different method than the old namespaced_key
which expanded, sanitized, namespaced, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
begin | ||
require "redis" | ||
require "redis/distributed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always require redis/distributed
, even if we use a single Redis, so we can reference Redis::Distributed::CannotDistribute
below.
begin | ||
require "redis/connection/hiredis" | ||
rescue LoadError | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's dubious. Maybe it's good practice. Maybe it's …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy Do I need to explicitly add gem 'hiredis-client'
or gem 'hiredis'
in my gemfile to use the hiredis
as a driver in Redis cache store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end | ||
|
||
require "digest/md5" | ||
require "active_support/core_ext/marshal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For large-key truncation and for entry marshaling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
md5 can be problematic; while it is slower, I think we should probably just use sha2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I used MD5 to match MemCacheStore. I'd like to flip them both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to SHA2. MemCacheStore can catch up later.
else | ||
super | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same story here, assuming #mset
-capable and falling back if not.
else | ||
key | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth hoisting this out too.
|
||
def failsafe(method, returning: nil) | ||
yield | ||
rescue ::Redis::BaseConnectionError => e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to expand the set of failsafe exceptions, and allow Rails users to do it.
end | ||
rescue => failsafe | ||
warn "RedisCacheStore ignored exception in handle_exception: #{failsafe.class}: #{failsafe.message}\n #{failsafe.backtrace.join("\n ")}" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exceptions are swallowed by default. Apps provide an error_handler
to report on them.
|
||
test "write failure returns nil" do | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD
b1a47ad
to
a131987
Compare
|
||
Deployment note: Take care to use a *dedicated Redis cache* rather | ||
than pointing this at your existing Redis server. It won't cope well | ||
with mixed usage patterns and it won't expire cache entries by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this advice consistent with it defaulting to redis://localhost:6379/0
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially set this up without a default for that reason, but it felt needlessly fussy for dev/test. I think clearly, repeatedly directing users to deployment guidance is the important ingredient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should default to some non-nil expiry, and then people heeding the "how to use this at its best" guidance can turn that off at the same time they point to a dedicated instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that direction since it require an informed choice of Redis usage mode.
Thing is, by default, Redis doesn't expire keys when it hits max mem—it returns errors. For expiry alone to work, we'd need to use volatile-lru/ttl
max-mem policy instead of the default noeviction
.
That suggests that we configure a volatile/allkeys policy on the cache store as well. Then we can inspect maxmemory-policy
to satisfy ourselves that the server is on board with the user's intent.
end | ||
|
||
require "digest/md5" | ||
require "active_support/core_ext/marshal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
md5 can be problematic; while it is slower, I think we should probably just use sha2.
else | ||
super | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth checking this when we connect? I remember a rescue
-covered block used to be fairly expensive to enter, regardless of whether the exception ever occurred... but maybe my memory's out of date?
raise ArgumentError, "Only Redis glob strings are supported: #{matcher.inspect}" | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, but that would imply this method isn't "Cache Store API implementation".
# Removes expired entries. Not supported. | ||
def cleanup(options = nil) | ||
super | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is expiry not supported at all? Or is expiry not used by default, and then when it is, delegated to redis? (Meaning this method would be a no-op, but would be "working")
# For compat with the wexpected options hash argument. | ||
def write_entry(key, entry, options = nil) | ||
write_entry_kwargs key, entry, **options | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't these have identical arities? 😕
# Compression is enabled by default with a 1kB threshold, so cached | ||
# values larger than 1kB are automatically compressed. Disable by | ||
# passing `cache: false` or change the threshold by passing | ||
# `compress_threshold: 4.kilobytes`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does sound like a good idea, but I wonder whether it's up to the redis store to have that opinion. Seems more like it should be the global default (and then overridden for the in-memory store)?
.travis.yml
Outdated
@@ -41,7 +42,7 @@ before_script: | |||
# Decodes to e.g. `export VARIABLE=VALUE` | |||
- $(base64 --decode <<< "ZXhwb3J0IFNBVUNFX0FDQ0VTU19LRVk9YTAzNTM0M2YtZTkyMi00MGIzLWFhM2MtMDZiM2VhNjM1YzQ4") | |||
- $(base64 --decode <<< "ZXhwb3J0IFNBVUNFX1VTRVJOQU1FPXJ1YnlvbnJhaWxz") | |||
- redis-server --bind 127.0.0.1 --port 6379 --requirepass 'password' --daemonize yes | |||
- redis-server --bind 127.0.0.1 --port 6380 --requirepass 'password' --daemonize yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have the test suite spin this up & down directly? Having to set a password on my local redis to get the tests to pass was annoying, but making everyone manually run a second instance seems even worse.
a6aa479
to
1c0315e
Compare
2c5b5cf
to
bf7b7d7
Compare
"#{namespace}:#{key}" | ||
else | ||
key | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was removed from public API in Rails 5.0. #22215 for context.
require "redis" | ||
require "redis/distributed" | ||
rescue LoadError | ||
warn "The Redis cache store requires the redis gem. Please add it to your Gemfile." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be doing gem 'redis
in this file so we assert the minimum version requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I haven't carefully reviewed the oldest supported version yet. Will probably be Redis 3.3.0 with support for connect/read/write_timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, requiring latest redis-rb (4.0.1+) is appropriate for a new feature like this. Then sharded cache read_multi
will just work without needing additional warnings.
bf7b7d7
to
00a59bd
Compare
"#{namespace}:#{key}" | ||
else | ||
key | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* Supports vanilla Redis, hiredis, and Redis::Distributed. * Supports Memcached-like sharding across Redises with Redis::Distributed. * Fault tolerant. If the Redis server is unavailable, no exceptions are raised. Cache fetches are treated as misses and writes are dropped. * Local cache. Hot in-memory primary cache within block/middleware scope. * `read_/write_multi` support for Redis mget/mset. Use Redis::Distributed 4.0.1+ for distributed mget support. * `delete_matched` support for Redis KEYS globs.
00a59bd
to
31f51b3
Compare
Thanks for review, @matthewd @rafaelfranca 🖖🏼 |
I'm a bit disappointed to see this from-scratch implementation considering all the effort that was put into https://github.com/sorentwo/readthis. It already supports every feature this implements, aside from distributed redis and local cache support. Porting Readthis would have been a great way to get something that is well documented, tested, and has been used in production for years into Rails. |
I have a question about the syntax of the configuration, raised at https://stackoverflow.com/questions/48177606/ruby-syntax-error-unexpected-tlabel I didn't think it was appropriate to discuss the question directly here, but hopefully ok to link to it, in case someone here can shed some light on it :-) |
@jkburges No biggie: it's not a function but an assignment. Surround the keys with braces. Should work. |
its built in to rails now, see See rails/rails#31134 as well as https://bigbinary.com/blog/rails-5.2-adds-write_multi-for-cache-writes
read_/write_multi
support for Redis mget/mset. Use Redis::Distributed 4.0.1+ for distributed mget support.delete_matched
support for Redis KEYS globs.Deployment note: Take care to use a dedicated Redis cache rather than pointing this at your existing Redis server. It won't cope well with mixed usage patterns and it won't expire cache entries by default.
Redis cache server setup guide: https://redis.io/topics/lru-cache