Closed jdelStrother closed 1 year ago
Any opinions?
Adding a method specific to Distributed
is a big no no. Both classes are supposed to be (mostly) interchangeable.
That some things break when you change from Redis
to Redis::Distributed
is a fact, but the inverse should always work.
Another option would be to set it as an option on the client (eg Redis::Distributed.new(node_configs, allow_nonatomic_operations: true)))
That wouldn't be the worse option.
Another is just to stop using Redis::Distributed
and have RedisCacheStore
do it's own distribution.
The common annoying problem with Redis is that it's both used as an ephemeral cache and as some persisted store, so you never know which tradeoff to make in the clients.
Fair enough... I might try removing Redis::Distributed from RedisCacheStore. Is it ok for it to use Redis::HashRing for looking up the right node?
Is it ok for it to use Redis::HashRing for looking up the right node?
I'd rather copy the implementation, at least for now. Would avoid some bad surprises, it's not really public API and is a bit complicated.
Also ultimately I'd like to refactor RedisCacheStore
to be able to use redis-client
directly, so that would be a step in the right direction.
For what it's worth I started a POC of replacing Redis::Distributed with a new ShardedRedis class here - https://github.com/jdelStrother/rails/commit/3a560b013c1b95a85019b8dc45e84adbc48de0d4.
It's maybe a useful stepping-stone towards just using redis-client, but actually I the main bit I'm interested in is improving multi_write with multiple redis nodes, and I think that could be done with without needing a new ShardedRedis class. We'd just need to tweak the RedisCacheStore#pipelined method to something like this - https://github.com/jdelStrother/rails/commit/3a560b013c1b95a85019b8dc45e84adbc48de0d4#diff-8fbbb6f4480a782f49dbae5d364199ea4242fff5f6bd36c3bb01e36bed060b09L297-L306
Unless you have strong opinions I might put the ShardedRedis stuff aside for now, and just try & get the pipelined improvements merged.
just try & get the pipelined improvements merged.
Sure. Not quite certain what you have in mind, but feel free to ping once you got a PR.
if you don't mind I'll close this one.
Redis::Distributed#mset
can't be guaranteed to be atomic, so it raises CannotDistribute by default.However, sometimes we don't care so much about atomicity (eg in Rails'
RedisCacheStore#write_multi
), so allow bypassing the CannotDistribute check.This is part of the fix for https://github.com/rails/rails/issues/48938
I'm not totally decided on the best way of doing this. I'd originally tried just adding an option to the original method -
but the kwargs make it tricky - I don't think it's possible to do that and still support
redis.mapped_mset({ foo: 1}, atomic: true)
andredis.mapped_mset(foo: 1)
.Another option would be to set it as an option on the client (eg
Redis::Distributed.new(node_configs, allow_nonatomic_operations: true))
), but I decided that was a bit weird since it only really makes sense for mset and mapped_mset.So for now I went with adding extra
mset_nonatomic
&mapped_mset_nonatomic
methods, but I could definitely be persuaded otherwise.We maybe also ought to consider what the fix in Rails will look like if & when this is fixed in redis-rb. I guess if we take the approach of this PR it'd be something like:
Any opinions?