chore: use hard coding instead of using command information (#362)

Author: supercaracal

lib/redis_client/cluster/command.rb

@@ -97,12 +97,6 @@ def exists?(name)
         @commands.key?(::RedisClient::Cluster::NormalizedCmdName.instance.get_by_name(name))
       end
 
-      def determine_key_step(command)
-        name = ::RedisClient::Cluster::NormalizedCmdName.instance.get_by_command(command)
-        # Some commands like EVALSHA have zero as the step in COMMANDS somehow.
-        @commands[name].key_step == 0 ? 1 : @commands[name].key_step
-      end
-
       private
 
       def determine_first_key_position(command) # rubocop:disable Metrics/CyclomaticComplexity
@@ -153,6 +147,12 @@ def determine_optional_key_position(command, option_name) # rubocop:disable Metr
         idx = command&.flatten&.map(&:to_s)&.map(&:downcase)&.index(option_name&.downcase)
         idx.nil? ? 0 : idx + 1
       end
+
+      def determine_key_step(command)
+        name = ::RedisClient::Cluster::NormalizedCmdName.instance.get_by_command(command)
+        # Some commands like EVALSHA have zero as the step in COMMANDS somehow.
+        @commands[name].key_step == 0 ? 1 : @commands[name].key_step
+      end
     end
   end
 end

lib/redis_client/cluster/router.rb

@@ -18,11 +18,6 @@ class Cluster
     class Router
       ZERO_CURSOR_FOR_SCAN = '0'
       TSF = ->(f, x) { f.nil? ? x : f.call(x) }.curry
-      MULTIPLE_KEYS_COMMAND_TO_PIPELINE = {
-        'mset' => 'set',
-        'mget' => 'get',
-        'del' => 'del'
-      }.freeze
 
       def initialize(config, concurrent_worker, pool: nil, **kwargs)
         @config = config.dup
@@ -334,27 +329,31 @@ def send_watch_command(command)
         end
       end
 
+      MULTIPLE_KEYS_COMMAND_TO_SINGLE = {
+        'mget' => ['get', 1].freeze,
+        'mset' => ['set', 2].freeze,
+        'del' => ['del', 1].freeze
+      }.freeze
+
       def send_multiple_keys_command(cmd, method, command, args, &block) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
-        key_step = @command.determine_key_step(cmd)
-        if command.size <= key_step + 1 || ::RedisClient::Cluster::KeySlotConverter.hash_tag_included?(command[1]) # rubocop:disable Style/IfUnlessModifier
-          return try_send(assign_node(command), method, command, args, &block)
-        end
+        # This implementation is prioritized performance rather than readability or so.
+        single_key_cmd, keys_step = MULTIPLE_KEYS_COMMAND_TO_SINGLE.fetch(cmd)
+
+        return try_send(assign_node(command), method, command, args, &block) if command.size <= keys_step + 1 || ::RedisClient::Cluster::KeySlotConverter.hash_tag_included?(command[1])
 
         seed = @config.use_replica? && @config.replica_affinity == :random ? nil : Random.new_seed
         pipeline = ::RedisClient::Cluster::Pipeline.new(self, @command_builder, @concurrent_worker, exception: true, seed: seed)
 
-        # This implementation is prioritized to lessen memory consumption rather than readability.
-        single_key_cmd = MULTIPLE_KEYS_COMMAND_TO_PIPELINE[cmd]
-        single_command = Array.new(key_step + 1)
+        single_command = Array.new(keys_step + 1)
         single_command[0] = single_key_cmd
-        if key_step == 1
+        if keys_step == 1
           command[1..].each do |key|
             single_command[1] = key
             pipeline.call_v(single_command)
           end
         else
-          command[1..].each_slice(key_step) do |v|
-            key_step.times { |i| single_command[i + 1] = v[i] }
+          command[1..].each_slice(keys_step) do |v|
+            keys_step.times { |i| single_command[i + 1] = v[i] }
             pipeline.call_v(single_command)
           end
         end

test/redis_client/cluster/test_command.rb

@@ -147,20 +147,6 @@ def test_exists?
         end
       end
 
-      def test_determine_key_step
-        cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
-        [
-          { name: 'MSET', want: 2 },
-          { name: 'MGET', want: 1 },
-          { name: 'DEL', want: 1 },
-          { name: 'EVALSHA', want: 1 }
-        ].each_with_index do |c, idx|
-          msg = "Case: #{idx}"
-          got = cmd.determine_key_step(c[:name])
-          assert_equal(c[:want], got, msg)
-        end
-      end
-
       def test_determine_first_key_position
         cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
         [
@@ -205,6 +191,20 @@ def test_determine_optional_key_position
         end
       end
 
+      def test_determine_key_step
+        cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
+        [
+          { name: 'MSET', want: 2 },
+          { name: 'MGET', want: 1 },
+          { name: 'DEL', want: 1 },
+          { name: 'EVALSHA', want: 1 }
+        ].each_with_index do |c, idx|
+          msg = "Case: #{idx}"
+          got = cmd.send(:determine_key_step, c[:name])
+          assert_equal(c[:want], got, msg)
+        end
+      end
+
       def test_extract_all_keys
         cmd = ::RedisClient::Cluster::Command.load(@raw_clients)
         [

test/redis_client/test_cluster.rb

@@ -207,6 +207,8 @@ def test_transaction_with_single_key
         end
 
         assert_equal(['OK', 1, 2], got)
+
+        wait_for_replication
         assert_equal('2', @client.call('GET', 'counter'))
       end
 
@@ -284,6 +286,8 @@ def test_transaction_with_hashtag
         end
 
         assert_equal(%w[OK OK], got)
+
+        wait_for_replication
         assert_equal(%w[1 2 3 4], @client.call('MGET', '{key}1', '{key}2', '{key}3', '{key}4'))
       end
 
@@ -319,6 +323,8 @@ def test_transaction_with_watch
         end
 
         assert_equal(%w[START OK OK FINISH], got)
+
+        wait_for_replication
         assert_equal(%w[1 2], @client.call('MGET', '{key}1', '{key}2'))
       end
 
@@ -339,6 +345,7 @@ def test_transaction_with_unsafe_watch
           end
         end
 
+        wait_for_replication
         assert_equal(%w[0 0], @client.call('MGET', '{key}1', '{key}2'))
       end
 
@@ -353,6 +360,8 @@ def test_transaction_with_meaningless_watch
         end
 
         assert_equal(%w[START OK OK FINISH], got)
+
+        wait_for_replication
         assert_equal(%w[1 2], @client.call('MGET', '{key}1', '{key}2'))
       end
 
@@ -392,6 +401,7 @@ def test_transaction_does_not_unwatch_on_connection_error
             tx.call('QUIT')
           end
         end
+
         command_list = @captured_commands.to_a.map(&:command).map(&:first)
         assert_includes(command_list, 'WATCH')
         refute_includes(command_list, 'UNWATCH')
@@ -421,6 +431,7 @@ def test_transaction_does_not_retry_without_rewatching
         end
 
         # The transaction did not commit.
+        wait_for_replication
         assert_equal('client2_value', @client.call('GET', 'key'))
       end
 
@@ -449,6 +460,7 @@ def test_transaction_with_watch_retries_block
         end
 
         # The transaction did commit (but it was the second time)
+        wait_for_replication
         assert_equal('@client_value_2', @client.call('GET', 'key'))
         assert_equal(2, call_count)
       end
@@ -463,6 +475,7 @@ def test_transaction_with_error
           end
         end
 
+        wait_for_replication
         assert_equal('x', @client.call('GET', 'key1'))
       end
 
@@ -476,6 +489,7 @@ def test_transaction_without_error_during_queueing
           end
         end
 
+        wait_for_replication
         assert_equal('aaa', @client.call('GET', 'key1'))
       end
 
@@ -518,6 +532,8 @@ def test_transaction_in_race_condition
         end
 
         assert_nil(got)
+
+        wait_for_replication
         assert_equal(%w[3 4], @client.call('MGET', '{key}1', '{key}2'))
       end
 
@@ -532,6 +548,8 @@ def test_transaction_with_dedicated_watch_command
         end
 
         assert_equal(%w[START OK OK FINISH], got)
+
+        wait_for_replication
         assert_equal(%w[1 2], @client.call('MGET', '{key}1', '{key}2'))
       end