From 119974758dd1bf1d69b7ab72c0125929ba58fe75 Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Sun, 17 Nov 2024 22:05:22 +0900 Subject: [PATCH 1/2] refactor: remove redundant and unused codes --- lib/redis_client/cluster/command.rb | 52 ++------------------- lib/redis_client/cluster/errors.rb | 5 ++- lib/redis_client/cluster/router.rb | 21 +++++---- test/redis_client/cluster/test_command.rb | 55 ++--------------------- 4 files changed, 22 insertions(+), 111 deletions(-) diff --git a/lib/redis_client/cluster/command.rb b/lib/redis_client/cluster/command.rb index 3664300..6f68bff 100644 --- a/lib/redis_client/cluster/command.rb +++ b/lib/redis_client/cluster/command.rb @@ -71,18 +71,7 @@ def extract_first_key(command) i = determine_first_key_position(command) return EMPTY_STRING if i == 0 - (command[i].is_a?(Array) ? command[i].flatten.first : command[i]).to_s - end - - def extract_all_keys(command) - keys_start = determine_first_key_position(command) - keys_end = determine_last_key_position(command, keys_start) - keys_step = determine_key_step(command) - return EMPTY_ARRAY if [keys_start, keys_end, keys_step].any?(&:zero?) - - keys_end = [keys_end, command.size - 1].min - # use .. inclusive range because keys_end is a valid index. - (keys_start..keys_end).step(keys_step).map { |i| command[i] } + command[i] end def should_send_to_primary?(command) @@ -116,45 +105,10 @@ def determine_first_key_position(command) # rubocop:disable Metrics/CyclomaticCo end end - # IMPORTANT: this determines the last key position INCLUSIVE of the last key - - # i.e. command[determine_last_key_position(command)] is a key. - # This is in line with what Redis returns from COMMANDS. - def determine_last_key_position(command, keys_start) # rubocop:disable Metrics/AbcSize - case name = ::RedisClient::Cluster::NormalizedCmdName.instance.get_by_command(command) - when 'eval', 'evalsha', 'zinterstore', 'zunionstore' - # EVALSHA sha1 numkeys [key [key ...]] [arg [arg ...]] - # ZINTERSTORE destination numkeys key [key ...] [WEIGHTS weight [weight ...]] [AGGREGATE ] - command[2].to_i + 2 - when 'object', 'memory' - # OBJECT [ENCODING | FREQ | IDLETIME | REFCOUNT] key - # MEMORY USAGE key [SAMPLES count] - keys_start - when 'migrate' - # MIGRATE host port destination-db timeout [COPY] [REPLACE] [AUTH password | AUTH2 username password] [KEYS key [key ...]] - command[3].empty? ? (command.length - 1) : 3 - when 'xread', 'xreadgroup' - # XREAD [COUNT count] [BLOCK milliseconds] STREAMS key [key ...] id [id ...] - keys_start + ((command.length - keys_start) / 2) - 1 - else - # If there is a fixed, non-variable number of keys, don't iterate past that. - if @commands[name].last_key_position >= 0 - @commands[name].last_key_position - else - command.length + @commands[name].last_key_position - end - end - end - - def determine_optional_key_position(command, option_name) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity - idx = command&.flatten&.map(&:to_s)&.map(&:downcase)&.index(option_name&.downcase) + def determine_optional_key_position(command, option_name) + idx = command.map { |e| e.to_s.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 diff --git a/lib/redis_client/cluster/errors.rb b/lib/redis_client/cluster/errors.rb index 72c79b9..a0be5f2 100644 --- a/lib/redis_client/cluster/errors.rb +++ b/lib/redis_client/cluster/errors.rb @@ -34,11 +34,14 @@ def self.from_command(command) end class ErrorCollection < Error + EMPTY_HASH = {}.freeze + + private_constant :EMPTY_HASH attr_reader :errors def self.with_errors(errors) if !errors.is_a?(Hash) || errors.empty? - new(errors.to_s).with_errors({}) + new(errors.to_s).with_errors(EMPTY_HASH) else messages = errors.map { |node_key, error| "#{node_key}: (#{error.class}) #{error.message}" } new(messages.join(', ')).with_errors(errors) diff --git a/lib/redis_client/cluster/router.rb b/lib/redis_client/cluster/router.rb index 0cd9076..8029480 100644 --- a/lib/redis_client/cluster/router.rb +++ b/lib/redis_client/cluster/router.rb @@ -367,17 +367,20 @@ 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 - - private_constant :MULTIPLE_KEYS_COMMAND_TO_SINGLE - def send_multiple_keys_command(cmd, method, command, args, &block) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity # This implementation is prioritized performance rather than readability or so. - single_key_cmd, keys_step = MULTIPLE_KEYS_COMMAND_TO_SINGLE.fetch(cmd) + case cmd + when 'mget' + single_key_cmd = 'get' + keys_step = 1 + when 'mset' + single_key_cmd = 'set' + keys_step = 2 + when 'del' + single_key_cmd = 'del' + keys_step = 1 + else raise NotImplementedError, cmd + end return try_send(assign_node(command), method, command, args, &block) if command.size <= keys_step + 1 || ::RedisClient::Cluster::KeySlotConverter.hash_tag_included?(command[1]) diff --git a/test/redis_client/cluster/test_command.rb b/test/redis_client/cluster/test_command.rb index 3c68003..cbca5eb 100644 --- a/test/redis_client/cluster/test_command.rb +++ b/test/redis_client/cluster/test_command.rb @@ -87,8 +87,6 @@ def test_extract_first_key { command: %w[GET foo{bar}baz], want: 'foo{bar}baz' }, { command: %w[MGET foo bar baz], want: 'foo' }, { command: %w[UNKNOWN foo bar], want: '' }, - { command: [['GET'], 'foo'], want: 'foo' }, - { command: ['GET', ['foo']], want: 'foo' }, { command: [], want: '' }, { command: nil, want: '' } ].each_with_index do |c, idx| @@ -151,7 +149,6 @@ def test_determine_first_key_position cmd = ::RedisClient::Cluster::Command.load(@raw_clients) [ { command: %w[EVAL "return ARGV[1]" 0 hello], want: 3 }, - { command: [['EVAL'], '"return ARGV[1]"', 0, 'hello'], want: 3 }, { command: %w[EVALSHA sha1 2 foo bar baz zap], want: 3 }, { command: %w[MIGRATE host port key 0 5 COPY], want: 3 }, { command: ['MIGRATE', 'host', 'port', '', '0', '5', 'COPY', 'KEYS', 'key'], want: 8 }, @@ -164,7 +161,7 @@ def test_determine_first_key_position { command: %w[XREADGROUP GROUP group consumer STREAMS key id], want: 5 }, { command: %w[SET foo 1], want: 1 }, { command: %w[set foo 1], want: 1 }, - { command: [['SET'], 'foo', 1], want: 1 }, + { command: ['SET', 'foo', 1], want: 1 }, { command: %w[GET foo], want: 1 } ].each_with_index do |c, idx| msg = "Case: #{idx}" @@ -179,62 +176,16 @@ def test_determine_optional_key_position { params: { command: %w[XREAD COUNT 2 STREAMS mystream writers 0-0 0-0], option_name: 'streams' }, want: 4 }, { params: { command: %w[XREADGROUP GROUP group consumer STREAMS key id], option_name: 'streams' }, want: 5 }, { params: { command: %w[GET foo], option_name: 'bar' }, want: 0 }, - { params: { command: ['FOO', ['BAR'], 'BAZ'], option_name: 'bar' }, want: 2 }, + { params: { command: %w[FOO BAR BAZ], option_name: 'bar' }, want: 2 }, { params: { command: %w[FOO BAR BAZ], option_name: 'BAR' }, want: 2 }, { params: { command: [], option_name: nil }, want: 0 }, - { params: { command: [], option_name: '' }, want: 0 }, - { params: { command: nil, option_name: nil }, want: 0 } + { params: { command: [], option_name: '' }, want: 0 } ].each_with_index do |c, idx| msg = "Case: #{idx}" got = cmd.send(:determine_optional_key_position, c[:params][:command], c[:params][:option_name]) assert_equal(c[:want], got, msg) 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) - [ - { command: ['EVAL', 'return ARGV[1]', '0', 'hello'], want: [] }, - { command: ['EVAL', 'return ARGV[1]', '3', 'key1', 'key2', 'key3', 'arg1', 'arg2'], want: %w[key1 key2 key3] }, - { command: [['EVAL'], '"return ARGV[1]"', 0, 'hello'], want: [] }, - { command: %w[EVALSHA sha1 2 foo bar baz zap], want: %w[foo bar] }, - { command: %w[MIGRATE host port key 0 5 COPY], want: %w[key] }, - { command: ['MIGRATE', 'host', 'port', '', '0', '5', 'COPY', 'KEYS', 'key1'], want: %w[key1] }, - { command: ['MIGRATE', 'host', 'port', '', '0', '5', 'COPY', 'KEYS', 'key1', 'key2'], want: %w[key1 key2] }, - { command: %w[ZINTERSTORE out 2 zset1 zset2 WEIGHTS 2 3], want: %w[zset1 zset2] }, - { command: %w[ZUNIONSTORE out 2 zset1 zset2 WEIGHTS 2 3], want: %w[zset1 zset2] }, - { command: %w[OBJECT HELP], want: [] }, - { command: %w[MEMORY HELP], want: [] }, - { command: %w[MEMORY USAGE key], want: %w[key] }, - { command: %w[XREAD COUNT 2 STREAMS mystream writers 0-0 0-0], want: %w[mystream writers] }, - { command: %w[XREADGROUP GROUP group consumer STREAMS key id], want: %w[key] }, - { command: %w[SET foo 1], want: %w[foo] }, - { command: %w[set foo 1], want: %w[foo] }, - { command: [['SET'], 'foo', 1], want: %w[foo] }, - { command: %w[GET foo], want: %w[foo] }, - { command: %w[MGET foo bar baz], want: %w[foo bar baz] }, - { command: %w[MSET foo val bar val baz val], want: %w[foo bar baz] }, - { command: %w[BLPOP foo bar 0], want: %w[foo bar] } - ].each_with_index do |c, idx| - msg = "Case: #{idx}" - got = cmd.send(:extract_all_keys, c[:command]) - assert_equal(c[:want], got, msg) - end - end end end end From 6e6c228eb0cc18f37ef4e6760b27bb61b7e74341 Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Sun, 17 Nov 2024 22:09:19 +0900 Subject: [PATCH 2/2] fix --- lib/redis_client/cluster/command.rb | 2 -- test/redis_client/cluster/test_command.rb | 6 +++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/redis_client/cluster/command.rb b/lib/redis_client/cluster/command.rb index 6f68bff..7db0a70 100644 --- a/lib/redis_client/cluster/command.rb +++ b/lib/redis_client/cluster/command.rb @@ -17,7 +17,6 @@ class Command Detail = Struct.new( 'RedisCommand', :first_key_position, - :last_key_position, :key_step, :write?, :readonly?, @@ -54,7 +53,6 @@ def parse_command_reply(rows) acc[row[0].downcase] = ::RedisClient::Cluster::Command::Detail.new( first_key_position: row[3], - last_key_position: row[4], key_step: row[5], write?: row[2].include?('write'), readonly?: row[2].include?('readonly') diff --git a/test/redis_client/cluster/test_command.rb b/test/redis_client/cluster/test_command.rb index cbca5eb..1234fe3 100644 --- a/test/redis_client/cluster/test_command.rb +++ b/test/redis_client/cluster/test_command.rb @@ -53,8 +53,8 @@ def test_parse_command_reply ['set', -3, Set['write', 'denyoom', 'movablekeys'], 1, -1, 2, Set['@write', '@string', '@slow'], Set[], Set[], Set[]] ], want: { - 'get' => { first_key_position: 1, last_key_position: -1, key_step: 1, write?: false, readonly?: true }, - 'set' => { first_key_position: 1, last_key_position: -1, key_step: 2, write?: true, readonly?: false } + 'get' => { first_key_position: 1, key_step: 1, write?: false, readonly?: true }, + 'set' => { first_key_position: 1, key_step: 2, write?: true, readonly?: false } } }, { @@ -62,7 +62,7 @@ def test_parse_command_reply ['GET', 2, Set['readonly', 'fast'], 1, -1, 1, Set['@read', '@string', '@fast'], Set[], Set[], Set[]] ], want: { - 'get' => { first_key_position: 1, last_key_position: -1, key_step: 1, write?: false, readonly?: true } + 'get' => { first_key_position: 1, key_step: 1, write?: false, readonly?: true } } }, { rows: [[]], want: {} },