Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove redundant or unused codes #407

Merged
merged 2 commits into from
Nov 17, 2024
Merged

refactor: remove redundant or unused codes #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1199747
refactor: remove redundant and unused codes
supercaracal Nov 17, 2024
6e6c228
fix
supercaracal Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 3 additions & 51 deletions lib/redis_client/cluster/command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class Command
Detail = Struct.new(
'RedisCommand',
:first_key_position,
:last_key_position,
:key_step,
:write?,
:readonly?,
Expand Down Expand Up @@ -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')
Expand All @@ -71,18 +69,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)
Expand Down Expand Up @@ -116,45 +103,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 <SUM | MIN | MAX>]
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 <key | ""> 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
5 changes: 4 additions & 1 deletion lib/redis_client/cluster/errors.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
21 changes: 12 additions & 9 deletions lib/redis_client/cluster/router.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])

Expand Down
61 changes: 6 additions & 55 deletions test/redis_client/cluster/test_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,16 @@ 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 }
}
},
{
rows: [
['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: {} },
Expand All @@ -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|
Expand Down Expand Up @@ -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 },
Expand All @@ -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}"
Expand All @@ -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