Merge pull request #1268 from supercaracal/fix-cluster-client

Add some documents for the transaction feature in the cluster client

cluster/README.md

@@ -75,3 +75,28 @@ Redis::Cluster.new(nodes: %w[rediss://foo-endpoint.example.com:6379], fixed_host
 ```
 
 In case of the above architecture, if you don't pass the `fixed_hostname` option to the client and servers return IP addresses of nodes, the client may fail to verify certificates.
+
+## Transaction with an optimistic locking
+Since Redis cluster is a distributed system, several behaviors are different from a standalone server.
+Client libraries can make them compatible up to a point, but a part of features needs some restrictions.
+Especially, some cautions are needed to use the transaction feature with an optimistic locking.
+
+```ruby
+redis.watch("{my}key") do |client|        # The client is an instance of the internal adapter
+  if redis.get("{my}key") == "some value" # We can't use the client passed by the block argument
+    client.multi do |tx|                  # The tx is the same instance of the internal adapter
+      tx.set("{my}key", "other value")
+      tx.incr("{my}counter")
+    end
+  else
+    client.unwatch
+  end
+end
+```
+
+In a cluster mode client, you need to pass a block if you call the watch method and you need to specify an argument to the block.
+Also, you should use the block argument as a receiver to call the transaction feature methods in the block.
+The commands called by methods of the receiver are added to the internal pipeline for the transaction and they are sent to the server lastly.
+On the other hand, if you want to call other methods for commands, you can use the global instance of the client instead of the block argument.
+It affects out of the transaction pipeline and the replies are returned soon.
+Although the above restrictions are needed, this implementations is compatible with a standalone client.

cluster/lib/redis/cluster.rb

@@ -96,18 +96,30 @@ def cluster(subcommand, *args)
       send_command([:cluster, subcommand] + args, &block)
     end
 
+    # Watch the given keys to determine execution of the MULTI/EXEC block.
+    #
+    # Using a block is required for a cluster client. It's different from a standalone client.
+    # And you should use the block argument as a receiver if you call transaction feature methods.
+    # On the other hand, you can use the global instance of the client if you call methods of other commands.
+    #
+    # An `#unwatch` is automatically issued if an exception is raised within the
+    # block that is a subclass of StandardError and is not a ConnectionError.
+    #
+    # @param keys [String, Array<String>] one or more keys to watch
+    # @return [Array<Object>] replies of the transaction or an empty array
+    #
     # @example A typical use case.
-    #   redis.watch("key") do |client|        # The client is an instance of the adapter
-    #     if redis.get("key") == "some value" # We can't use the client passed by the block argument
-    #       client.multi do |tx|              # The tx is the same instance of the adapter
-    #         tx.set("key", "other value")
-    #         tx.incr("counter")
+    #   redis.watch("{my}key") do |client|        # The client is an instance of the internal adapter
+    #     if redis.get("{my}key") == "some value" # We can't use the client passed by the block argument
+    #       client.multi do |tx|                  # The tx is the same instance of the internal adapter
+    #         tx.set("{my}key", "other value")
+    #         tx.incr("{my}counter")
     #       end
     #     else
     #       client.unwatch
     #     end
     #   end
-    #   # => ["OK", 6]
+    #   #=> ["OK", 6]
     def watch(*keys, &block)
       synchronize { |c| c.watch(*keys, &block) }
     end

cluster/lib/redis/cluster/client.rb

@@ -99,9 +99,19 @@ def multi(watch: nil, &block)
         handle_errors { super(watch: watch, &block) }
       end
 
-      def watch(*keys)
+      def watch(*keys, &block)
         unless block_given?
-          raise Redis::Cluster::TransactionConsistencyError, 'A block is required if you use the cluster client.'
+          raise(
+            Redis::Cluster::TransactionConsistencyError,
+            'A block is required if you use the cluster client.'
+          )
+        end
+
+        unless block.arity == 1
+          raise(
+            Redis::Cluster::TransactionConsistencyError,
+            'Given block needs an argument if you use the cluster client.'
+          )
         end
 
         handle_errors do

cluster/lib/redis/cluster/transaction_adapter.rb

@@ -23,7 +23,10 @@ def discard
       end
 
       def watch(*_)
-        # no need to do anything
+        raise(
+          Redis::Cluster::TransactionConsistencyError,
+          'You should pass all the keys to a watch method if you use the cluster client.'
+        )
       end
 
       def unwatch

cluster/test/commands_on_transactions_test.rb

@@ -40,6 +40,16 @@ def test_watch
       redis.watch('{key}1', '{key}2')
     end
 
+    assert_raises(Redis::Cluster::TransactionConsistencyError) do
+      redis.watch('{key}1', '{key}2') {}
+    end
+
+    assert_raises(Redis::Cluster::TransactionConsistencyError) do
+      redis.watch('{key}1', '{key}2') do |tx|
+        tx.watch('{key}3')
+      end
+    end
+
     assert_raises(Redis::Cluster::TransactionConsistencyError) do
       redis.watch('key1', 'key2') do |tx|
         tx.set('key1', '1')
@@ -54,7 +64,7 @@ def test_watch
       end
     end
 
-    assert_empty(redis.watch('{key}1', '{key}2') {})
+    assert_empty(redis.watch('{key}1', '{key}2') { |_| })
 
     redis.watch('{key}1', '{key}2') do |tx|
       tx.set('{key}1', '1')