From b65287304b78dfee518e7fd78f8b8237b4338426 Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Fri, 12 Apr 2024 17:47:26 +0900 Subject: [PATCH 1/6] fix: make the error identification optional because of the performance --- lib/redis_client/cluster/error_identification.rb | 8 +++++++- lib/redis_client/cluster/node.rb | 11 ++--------- lib/redis_client/cluster/router.rb | 9 ++++++--- test/redis_client/test_cluster.rb | 10 +++++----- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/redis_client/cluster/error_identification.rb b/lib/redis_client/cluster/error_identification.rb index 17ad8a09..bc150b0a 100644 --- a/lib/redis_client/cluster/error_identification.rb +++ b/lib/redis_client/cluster/error_identification.rb @@ -4,7 +4,13 @@ class RedisClient class Cluster module ErrorIdentification def self.client_owns_error?(err, client) - err.is_a?(TaggedError) && err.from?(client) + return true unless identifiable?(err) + + err.from?(client) + end + + def self.identifiable?(err) + err.is_a?(TaggedError) end module TaggedError diff --git a/lib/redis_client/cluster/node.rb b/lib/redis_client/cluster/node.rb index 66ff3936..e301bbec 100644 --- a/lib/redis_client/cluster/node.rb +++ b/lib/redis_client/cluster/node.rb @@ -2,7 +2,6 @@ require 'redis_client' require 'redis_client/config' -require 'redis_client/cluster/error_identification' require 'redis_client/cluster/errors' require 'redis_client/cluster/node/primary_only' require 'redis_client/cluster/node/random_replica' @@ -79,11 +78,9 @@ def []=(index, element) end class Config < ::RedisClient::Config - def initialize(scale_read: false, middlewares: nil, **kwargs) + def initialize(scale_read: false, **kwargs) @scale_read = scale_read - middlewares ||= [] - middlewares.unshift ErrorIdentification::Middleware - super(middlewares: middlewares, **kwargs) + super(**kwargs) end private @@ -217,10 +214,6 @@ def reload! end end - def owns_error?(err) - any? { |c| ErrorIdentification.client_owns_error?(err, c) } - end - private def make_topology_class(with_replica, replica_affinity) diff --git a/lib/redis_client/cluster/router.rb b/lib/redis_client/cluster/router.rb index 5583e48f..16675b94 100644 --- a/lib/redis_client/cluster/router.rb +++ b/lib/redis_client/cluster/router.rb @@ -10,6 +10,7 @@ require 'redis_client/cluster/normalized_cmd_name' require 'redis_client/cluster/transaction' require 'redis_client/cluster/optimistic_locking' +require 'redis_client/cluster/error_identification' class RedisClient class Cluster @@ -68,7 +69,9 @@ def send_command(method, command, *args, &block) # rubocop:disable Metrics/AbcSi raise if e.errors.any?(::RedisClient::CircuitBreaker::OpenCircuitError) update_cluster_info! if e.errors.values.any? do |err| - @node.owns_error?(err) && err.message.start_with?('CLUSTERDOWN Hash slot not served') + next false if ::RedisClient::Cluster::ErrorIdentification.identifiable?(err) && @node.none? { |c| ::RedisClient::Cluster::ErrorIdentification.client_owns_error?(err, c) } + + err.message.start_with?('CLUSTERDOWN Hash slot not served') end raise @@ -97,7 +100,7 @@ def handle_redirection(node, retry_count:) # rubocop:disable Metrics/AbcSize, Me rescue ::RedisClient::CircuitBreaker::OpenCircuitError raise rescue ::RedisClient::CommandError => e - raise unless ErrorIdentification.client_owns_error?(e, node) + raise unless ::RedisClient::Cluster::ErrorIdentification.client_owns_error?(e, node) if e.message.start_with?('MOVED') node = assign_redirection_node(e.message) @@ -117,7 +120,7 @@ def handle_redirection(node, retry_count:) # rubocop:disable Metrics/AbcSize, Me end raise rescue ::RedisClient::ConnectionError => e - raise unless ErrorIdentification.client_owns_error?(e, node) + raise unless ::RedisClient::Cluster::ErrorIdentification.client_owns_error?(e, node) update_cluster_info! diff --git a/test/redis_client/test_cluster.rb b/test/redis_client/test_cluster.rb index 0b8cead7..7339759a 100644 --- a/test/redis_client/test_cluster.rb +++ b/test/redis_client/test_cluster.rb @@ -814,7 +814,7 @@ def new_test_client(capture_buffer: @captured_commands, **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware], + middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], custom: { captured_commands: capture_buffer }, **TEST_GENERIC_OPTIONS, **opts @@ -832,7 +832,7 @@ def new_test_client(capture_buffer: @captured_commands, **opts) replica: true, replica_affinity: :random, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware], + middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], custom: { captured_commands: capture_buffer }, **TEST_GENERIC_OPTIONS, **opts @@ -850,7 +850,7 @@ def new_test_client(capture_buffer: @captured_commands, **opts) replica: true, replica_affinity: :random_with_primary, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware], + middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], custom: { captured_commands: capture_buffer }, **TEST_GENERIC_OPTIONS, **opts @@ -868,7 +868,7 @@ def new_test_client(capture_buffer: @captured_commands, **opts) replica: true, replica_affinity: :latency, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware], + middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], custom: { captured_commands: capture_buffer }, **TEST_GENERIC_OPTIONS, **opts @@ -884,7 +884,7 @@ def new_test_client(capture_buffer: @captured_commands, **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware], + middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], custom: { captured_commands: capture_buffer }, **TEST_GENERIC_OPTIONS, **opts From eb4b34201fe69750c7a804e5c47683105f48d2b0 Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Fri, 12 Apr 2024 18:45:55 +0900 Subject: [PATCH 2/6] wip --- test/redirection_emulation_middleware.rb | 17 +++++++ test/redis_client/test_cluster.rb | 58 +++++++++++++++--------- test/testing_helper.rb | 1 + 3 files changed, 54 insertions(+), 22 deletions(-) create mode 100644 test/redirection_emulation_middleware.rb diff --git a/test/redirection_emulation_middleware.rb b/test/redirection_emulation_middleware.rb new file mode 100644 index 00000000..531b885c --- /dev/null +++ b/test/redirection_emulation_middleware.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module RedirectionEmulationMiddleware + def call(cmd, cfg) + r = cfg.custom[:redirect] + raise RedisClient::CommandError, "MOVED #{r[:slot]} #{r[:to]}" if cmd == r[:command] + + super + end + + def call_pipelined(cmd, cfg) + r = cfg.custom[:redirect] + raise RedisClient::CommandError, "MOVED #{r[:slot]} #{r[:to]}" if cmd == r[:command] + + super + end +end diff --git a/test/redis_client/test_cluster.rb b/test/redis_client/test_cluster.rb index 7339759a..af8ad946 100644 --- a/test/redis_client/test_cluster.rb +++ b/test/redis_client/test_cluster.rb @@ -717,21 +717,35 @@ def test_circuit_breakers end def test_only_reshards_own_errors - @client.call_v(%w[SADD testkey testvalue1]) - @client.call_v(%w[SADD testkey testvalue2]) slot = ::RedisClient::Cluster::KeySlotConverter.convert('testkey') router = @client.instance_variable_get(:@router) correct_primary_key = router.find_node_key_by_key('testkey', primary: true) broken_primary_key = (router.node_keys - [correct_primary_key]).first + + client1 = new_test_client( + middlewares: [::RedisClient::Cluster::ErrorIdentification::Middleware] + ) + + client2 = new_test_client( + middlewares: [RedirectionEmulationMiddleware], + custom: { redirect: { slot: slot, to: broken_primary_key, command: %w[GET testkey] } } + ) + assert_raises(RedisClient::CommandError) do - @client.sscan('testkey', retry_count: 0) do - raise RedisClient::CommandError, "MOVED #{slot} #{broken_primary_key}" + client1.call('GET', 'safekey') do + client2.call('GET', 'testkey') end end - # The exception should not have causes @client to update its shard mappings, because it didn't - # come from a RedisClient instance that @client knows about. - assert_equal correct_primary_key, router.find_node_key_by_key('testkey', primary: true) + # The exception should not have causes client to update its shard mappings, because it didn't + # come from a RedisClient instance that client knows about. + assert_equal( + correct_primary_key, + client1.instance_variable_get(:@router).find_node_key_by_key('testkey', primary: true) + ) + + client1.close + client2.close end def test_pinning_single_key @@ -810,12 +824,12 @@ def hiredis_used? class PrimaryOnly < TestingWrapper include Mixin - def new_test_client(capture_buffer: @captured_commands, **opts) + def new_test_client(custom: { captured_commands: @captured_commands }, middlewares: [CommandCaptureMiddleware], **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], - custom: { captured_commands: capture_buffer }, + middlewares: middlewares, + custom: custom, **TEST_GENERIC_OPTIONS, **opts ) @@ -826,14 +840,14 @@ def new_test_client(capture_buffer: @captured_commands, **opts) class ScaleReadRandom < TestingWrapper include Mixin - def new_test_client(capture_buffer: @captured_commands, **opts) + def new_test_client(custom: { captured_commands: @captured_commands }, middlewares: [CommandCaptureMiddleware], **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, replica: true, replica_affinity: :random, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], - custom: { captured_commands: capture_buffer }, + middlewares: middlewares, + custom: custom, **TEST_GENERIC_OPTIONS, **opts ) @@ -844,14 +858,14 @@ def new_test_client(capture_buffer: @captured_commands, **opts) class ScaleReadRandomWithPrimary < TestingWrapper include Mixin - def new_test_client(capture_buffer: @captured_commands, **opts) + def new_test_client(custom: { captured_commands: @captured_commands }, middlewares: [CommandCaptureMiddleware], **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, replica: true, replica_affinity: :random_with_primary, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], - custom: { captured_commands: capture_buffer }, + middlewares: middlewares, + custom: custom, **TEST_GENERIC_OPTIONS, **opts ) @@ -862,14 +876,14 @@ def new_test_client(capture_buffer: @captured_commands, **opts) class ScaleReadLatency < TestingWrapper include Mixin - def new_test_client(capture_buffer: @captured_commands, **opts) + def new_test_client(custom: { captured_commands: @captured_commands }, middlewares: [CommandCaptureMiddleware], **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, replica: true, replica_affinity: :latency, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], - custom: { captured_commands: capture_buffer }, + middlewares: middlewares, + custom: custom, **TEST_GENERIC_OPTIONS, **opts ) @@ -880,12 +894,12 @@ def new_test_client(capture_buffer: @captured_commands, **opts) class Pooled < TestingWrapper include Mixin - def new_test_client(capture_buffer: @captured_commands, **opts) + def new_test_client(custom: { captured_commands: @captured_commands }, middlewares: [CommandCaptureMiddleware], **opts) config = ::RedisClient::ClusterConfig.new( nodes: TEST_NODE_URIS, fixed_hostname: TEST_FIXED_HOSTNAME, - middlewares: [CommandCaptureMiddleware, ::RedisClient::Cluster::ErrorIdentification::Middleware], - custom: { captured_commands: capture_buffer }, + middlewares: middlewares, + custom: custom, **TEST_GENERIC_OPTIONS, **opts ) diff --git a/test/testing_helper.rb b/test/testing_helper.rb index 4d09ff23..1cdf2450 100644 --- a/test/testing_helper.rb +++ b/test/testing_helper.rb @@ -7,6 +7,7 @@ require 'testing_constants' require 'cluster_controller' require 'command_capture_middleware' +require 'redirection_emulation_middleware' case ENV.fetch('REDIS_CONNECTION_DRIVER', 'ruby') when 'hiredis' then require 'hiredis-client' From a3858ce50715b999c41734616227501b8544d37b Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Fri, 12 Apr 2024 19:56:22 +0900 Subject: [PATCH 3/6] fix --- test/redis_client/test_cluster.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/redis_client/test_cluster.rb b/test/redis_client/test_cluster.rb index af8ad946..9c88a1cc 100644 --- a/test/redis_client/test_cluster.rb +++ b/test/redis_client/test_cluster.rb @@ -727,18 +727,24 @@ def test_only_reshards_own_errors ) client2 = new_test_client( - middlewares: [RedirectionEmulationMiddleware], + middlewares: [ + ::RedisClient::Cluster::ErrorIdentification::Middleware, + RedirectionEmulationMiddleware + ], custom: { redirect: { slot: slot, to: broken_primary_key, command: %w[GET testkey] } } ) + @client.call('SET', 'testkey', 'testvalue') + assert_raises(RedisClient::CommandError) do - client1.call('GET', 'safekey') do + client1.call('GET', 'testkey') do |got| + assert_equal('testvalue', got) client2.call('GET', 'testkey') end end - # The exception should not have causes client to update its shard mappings, because it didn't - # come from a RedisClient instance that client knows about. + # The exception should not have causes client1 to update its shard mappings, because it didn't + # come from a RedisClient instance that client1 knows about. assert_equal( correct_primary_key, client1.instance_variable_get(:@router).find_node_key_by_key('testkey', primary: true) From f63e903e26c0aea75e24290ecd15e6ccb649d3cc Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Fri, 12 Apr 2024 22:05:28 +0900 Subject: [PATCH 4/6] fix? --- test/redis_client/test_cluster.rb | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/redis_client/test_cluster.rb b/test/redis_client/test_cluster.rb index 9c88a1cc..b4624ae1 100644 --- a/test/redis_client/test_cluster.rb +++ b/test/redis_client/test_cluster.rb @@ -726,11 +726,20 @@ def test_only_reshards_own_errors middlewares: [::RedisClient::Cluster::ErrorIdentification::Middleware] ) + middlewares = [ + ::RedisClient::Cluster::ErrorIdentification::Middleware, + RedirectionEmulationMiddleware + ] + + if RUBY_ENGINE == 'ruby' + major, minor, = RUBY_VERSION.split('.') + major = Integer(major) + minor = Integer(minor) + middlewares.reverse! if major < 3 || (major >= 3 && minor < 1) + end + client2 = new_test_client( - middlewares: [ - ::RedisClient::Cluster::ErrorIdentification::Middleware, - RedirectionEmulationMiddleware - ], + middlewares: middlewares, custom: { redirect: { slot: slot, to: broken_primary_key, command: %w[GET testkey] } } ) From 089be76e0df50eb569906f52f6d915a2c886e627 Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Sat, 13 Apr 2024 13:44:39 +0900 Subject: [PATCH 5/6] flaky? --- test/redis_client/test_cluster.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/redis_client/test_cluster.rb b/test/redis_client/test_cluster.rb index 04dec1b7..d0762ed4 100644 --- a/test/redis_client/test_cluster.rb +++ b/test/redis_client/test_cluster.rb @@ -753,12 +753,12 @@ def test_only_reshards_own_errors RedirectionEmulationMiddleware ] - if RUBY_ENGINE == 'ruby' - major, minor, = RUBY_VERSION.split('.') - major = Integer(major) - minor = Integer(minor) - middlewares.reverse! if major < 3 || (major >= 3 && minor < 1) - end + # if RUBY_ENGINE == 'ruby' + # major, minor, = RUBY_VERSION.split('.') + # major = Integer(major) + # minor = Integer(minor) + # middlewares.reverse! if major < 3 || (major >= 3 && minor < 1) + # end client2 = new_test_client( middlewares: middlewares, From 3c1cd48a115e8c198ee30bad264b037aeb3040fd Mon Sep 17 00:00:00 2001 From: Taishi Kasuga Date: Sat, 13 Apr 2024 14:10:38 +0900 Subject: [PATCH 6/6] fix --- test/redirection_emulation_middleware.rb | 13 ++++++--- test/redis_client/test_cluster.rb | 35 ++++++++++-------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/test/redirection_emulation_middleware.rb b/test/redirection_emulation_middleware.rb index 531b885c..7e1091fa 100644 --- a/test/redirection_emulation_middleware.rb +++ b/test/redirection_emulation_middleware.rb @@ -1,16 +1,21 @@ # frozen_string_literal: true module RedirectionEmulationMiddleware + Setting = Struct.new( + 'RedirectionEmulationMiddlewareSetting', + :slot, :to, :command, keyword_init: true + ) + def call(cmd, cfg) - r = cfg.custom[:redirect] - raise RedisClient::CommandError, "MOVED #{r[:slot]} #{r[:to]}" if cmd == r[:command] + s = cfg.custom.fetch(:redirect) + raise RedisClient::CommandError, "MOVED #{s.slot} #{s.to}" if cmd == s.command super end def call_pipelined(cmd, cfg) - r = cfg.custom[:redirect] - raise RedisClient::CommandError, "MOVED #{r[:slot]} #{r[:to]}" if cmd == r[:command] + s = cfg.custom.fetch(:redirect) + raise RedisClient::CommandError, "MOVED #{s.slot} #{s.to}" if cmd == s.command super end diff --git a/test/redis_client/test_cluster.rb b/test/redis_client/test_cluster.rb index d0762ed4..2649c45c 100644 --- a/test/redis_client/test_cluster.rb +++ b/test/redis_client/test_cluster.rb @@ -745,32 +745,27 @@ def test_only_reshards_own_errors broken_primary_key = (router.node_keys - [correct_primary_key]).first client1 = new_test_client( - middlewares: [::RedisClient::Cluster::ErrorIdentification::Middleware] + middlewares: [ + ::RedisClient::Cluster::ErrorIdentification::Middleware + ] ) - middlewares = [ - ::RedisClient::Cluster::ErrorIdentification::Middleware, - RedirectionEmulationMiddleware - ] - - # if RUBY_ENGINE == 'ruby' - # major, minor, = RUBY_VERSION.split('.') - # major = Integer(major) - # minor = Integer(minor) - # middlewares.reverse! if major < 3 || (major >= 3 && minor < 1) - # end - client2 = new_test_client( - middlewares: middlewares, - custom: { redirect: { slot: slot, to: broken_primary_key, command: %w[GET testkey] } } + middlewares: [ + ::RedisClient::Cluster::ErrorIdentification::Middleware, + RedirectionEmulationMiddleware + ], + custom: { + redirect: RedirectionEmulationMiddleware::Setting.new( + slot: slot, to: broken_primary_key, command: %w[SET testkey client2] + ) + } ) - @client.call('SET', 'testkey', 'testvalue') - assert_raises(RedisClient::CommandError) do - client1.call('GET', 'testkey') do |got| - assert_equal('testvalue', got) - client2.call('GET', 'testkey') + client1.call('SET', 'testkey', 'client1') do |got| + assert_equal('OK', got) + client2.call('SET', 'testkey', 'client2') end end