Add Redis::Distributed#mset_nonatomic & mapped_mset_nonatomic

[jdelStrother]

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 rails/rails#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 -

   def mapped_mset(hash, atomic: true)
      raise CannotDistribute, :mapped_mset if atomic

     group_hash_by_node(hash) do |node, subhash|
        node.mapped_mset(subhash)
      end
    end

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) and redis.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:

diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb
index 4a5a28a47d..88c900782b 100644
--- a/activesupport/lib/active_support/cache/redis_cache_store.rb
+++ b/activesupport/lib/active_support/cache/redis_cache_store.rb
@@ -311,15 +311,22 @@ def mset_capable? # :nodoc:
         @mset_capable
       end
 
+      def nonatomic_mset?
+        set_redis_capabilities unless defined? @nonatomic_mset
+        @nonatomic_mset
+      end
+
       private
         def set_redis_capabilities
           case redis
           when Redis::Distributed
             @mget_capable = true
             @mset_capable = false
+            @nonatonic_mset = redis.respond_to?(:mapped_mset_nonatomic)
           else
             @mget_capable = true
             @mset_capable = true
+            @nonatonic_mset = false
           end
         end
 
@@ -416,7 +423,7 @@ def write_multi_entries(entries, expires_in: nil, **options)
               failsafe :write_multi_entries do
                 payload = serialize_entries(entries, **options)
                 redis.with do |c|
-                  c.mapped_mset(payload)
+                  nonatomic_mset? ? c.mapped_mset_nonatomic(payload) : c.mapped_mset(payload)
                 end
               end
             else

Any opinions?

[byroot]

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.

[jdelStrother]

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?

[byroot]

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.

[jdelStrother]

For what it's worth I started a POC of replacing Redis::Distributed with a new ShardedRedis class here - jdelStrother/rails@3a560b0.

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 - jdelStrother/rails@3a560b0#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.

[byroot]

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.