Skip to content

Commit 97d0499

Browse files
author
KJ Tsanaktsidis
committed
Handle timeouts to blocking_v specially via a custom client class
try_send already handles the case where the call is a blocking one specially, but try_delegate does not. This diff detects whether or not a ::RedisClient::ReadTimeoutError is for a blocking call at the source, and wraps it in a special subclass so we can differentiate it.
1 parent 70c3056 commit 97d0499

File tree

8 files changed

+45
-26
lines changed

8 files changed

+45
-26
lines changed

‎lib/redis_client/cluster/errors.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,5 +55,7 @@ def initialize(_ = '')
5555
)
5656
end
5757
end
58+
59+
class BlockingReadTimeoutError < ::RedisClient::ReadTimeoutError; end
5860
end
5961
end

‎lib/redis_client/cluster/node.rb

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ def []=(index, element)
8080
class Config < ::RedisClient::Config
8181
def initialize(scale_read: false, **kwargs)
8282
@scale_read = scale_read
83-
super(**kwargs)
83+
super(**kwargs, client_implementation: Client)
8484
end
8585

8686
private
@@ -92,6 +92,22 @@ def build_connection_prelude
9292
end
9393
end
9494

95+
class Client < ::RedisClient
96+
# We need to be able to differentiate between timeout errors caused by blocking read timeouts
97+
# (which should NOT cause a cluster topology update) with normal read timeouts (which should)
98+
def blocking_call(timeout, *command, **kwargs)
99+
super
100+
rescue ::RedisClient::TimeoutError => e
101+
raise ::RedisClient::Cluster::BlockingReadTimeoutError, e.message
102+
end
103+
104+
def blocking_call_v(timeout, command)
105+
super
106+
rescue ::RedisClient::TimeoutError => e
107+
raise ::RedisClient::Cluster::BlockingReadTimeoutError, e.message
108+
end
109+
end
110+
95111
class << self
96112
def load_info(options, concurrent_worker, slow_command_timeout: -1, **kwargs) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
97113
raise ::RedisClient::Cluster::InitialSetupError, [] if options.nil? || options.empty?

‎lib/redis_client/cluster/router.rb

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ class RedisClient
1313
class Cluster
1414
class Router
1515
ZERO_CURSOR_FOR_SCAN = '0'
16-
METHODS_FOR_BLOCKING_CMD = %i[blocking_call_v blocking_call].freeze
1716
TSF = ->(f, x) { f.nil? ? x : f.call(x) }.curry
1817

1918
def initialize(config, concurrent_worker, pool: nil, **kwargs)
@@ -98,9 +97,9 @@ def try_send(node, method, command, args, retry_count: 3, &block) # rubocop:disa
9897
retry if retry_count >= 0
9998
end
10099
raise
100+
rescue ::RedisClient::Cluster::BlockingReadTimeoutError
101+
raise
101102
rescue ::RedisClient::ConnectionError => e
102-
raise if METHODS_FOR_BLOCKING_CMD.include?(method) && e.is_a?(RedisClient::ReadTimeoutError)
103-
104103
update_cluster_info!
105104

106105
raise if retry_count <= 0
@@ -129,6 +128,8 @@ def try_delegate(node, method, *args, retry_count: 3, **kwargs, &block) # ruboco
129128
retry if retry_count >= 0
130129
end
131130
raise
131+
rescue ::RedisClient::Cluster::BlockingReadTimeoutError
132+
raise
132133
rescue ::RedisClient::ConnectionError
133134
update_cluster_info!
134135

‎test/redis_client/cluster/node/test_latency_replica.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ class TestLatencyReplica < TestingWrapper
1111

1212
def test_clients_with_redis_client
1313
got = @test_topology.clients
14-
got.each_value { |client| assert_instance_of(::RedisClient, client) }
14+
got.each_value { |client| assert_kind_of(::RedisClient, client) }
1515
assert_equal(%w[master slave], got.map { |_, v| v.call('ROLE').first }.uniq.sort)
1616
end
1717

@@ -25,7 +25,7 @@ def test_clients_with_pooled_redis_client
2525
)
2626

2727
got = test_topology.clients
28-
got.each_value { |client| assert_instance_of(::RedisClient::Pooled, client) }
28+
got.each_value { |client| assert_kind_of(::RedisClient::Pooled, client) }
2929
assert_equal(%w[master slave], got.map { |_, v| v.call('ROLE').first }.uniq.sort)
3030
ensure
3131
test_topology&.clients&.each_value(&:close)
@@ -34,22 +34,22 @@ def test_clients_with_pooled_redis_client
3434
def test_primary_clients
3535
got = @test_topology.primary_clients
3636
got.each_value do |client|
37-
assert_instance_of(::RedisClient, client)
37+
assert_kind_of(::RedisClient, client)
3838
assert_equal('master', client.call('ROLE').first)
3939
end
4040
end
4141

4242
def test_replica_clients
4343
got = @test_topology.replica_clients
4444
got.each_value do |client|
45-
assert_instance_of(::RedisClient, client)
45+
assert_kind_of(::RedisClient, client)
4646
assert_equal('slave', client.call('ROLE').first)
4747
end
4848
end
4949

5050
def test_clients_for_scanning
5151
got = @test_topology.clients_for_scanning
52-
got.each_value { |client| assert_instance_of(::RedisClient, client) }
52+
got.each_value { |client| assert_kind_of(::RedisClient, client) }
5353
assert_equal(TEST_SHARD_SIZE, got.size)
5454
end
5555

‎test/redis_client/cluster/node/test_primary_only.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ class TestPrimaryOnly < TestingWrapper
1212
def test_clients_with_redis_client
1313
got = @test_topology.clients
1414
got.each_value do |client|
15-
assert_instance_of(::RedisClient, client)
15+
assert_kind_of(::RedisClient, client)
1616
assert_equal('master', client.call('ROLE').first)
1717
end
1818
end
@@ -28,7 +28,7 @@ def test_clients_with_pooled_redis_client
2828

2929
got = test_topology.clients
3030
got.each_value do |client|
31-
assert_instance_of(::RedisClient::Pooled, client)
31+
assert_kind_of(::RedisClient::Pooled, client)
3232
assert_equal('master', client.call('ROLE').first)
3333
end
3434
ensure
@@ -38,23 +38,23 @@ def test_clients_with_pooled_redis_client
3838
def test_primary_clients
3939
got = @test_topology.primary_clients
4040
got.each_value do |client|
41-
assert_instance_of(::RedisClient, client)
41+
assert_kind_of(::RedisClient, client)
4242
assert_equal('master', client.call('ROLE').first)
4343
end
4444
end
4545

4646
def test_replica_clients
4747
got = @test_topology.replica_clients
4848
got.each_value do |client|
49-
assert_instance_of(::RedisClient, client)
49+
assert_kind_of(::RedisClient, client)
5050
assert_equal('master', client.call('ROLE').first)
5151
end
5252
end
5353

5454
def test_clients_for_scanning
5555
got = @test_topology.clients_for_scanning
5656
got.each_value do |client|
57-
assert_instance_of(::RedisClient, client)
57+
assert_kind_of(::RedisClient, client)
5858
assert_equal('master', client.call('ROLE').first)
5959
end
6060
end

‎test/redis_client/cluster/node/test_random_replica.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ class TestRandomReplica < TestingWrapper
1111

1212
def test_clients_with_redis_client
1313
got = @test_topology.clients
14-
got.each_value { |client| assert_instance_of(::RedisClient, client) }
14+
got.each_value { |client| assert_kind_of(::RedisClient, client) }
1515
assert_equal(%w[master slave], got.map { |_, v| v.call('ROLE').first }.uniq.sort)
1616
end
1717

@@ -25,7 +25,7 @@ def test_clients_with_pooled_redis_client
2525
)
2626

2727
got = test_topology.clients
28-
got.each_value { |client| assert_instance_of(::RedisClient::Pooled, client) }
28+
got.each_value { |client| assert_kind_of(::RedisClient::Pooled, client) }
2929
assert_equal(%w[master slave], got.map { |_, v| v.call('ROLE').first }.uniq.sort)
3030
ensure
3131
test_topology&.clients&.each_value(&:close)
@@ -34,22 +34,22 @@ def test_clients_with_pooled_redis_client
3434
def test_primary_clients
3535
got = @test_topology.primary_clients
3636
got.each_value do |client|
37-
assert_instance_of(::RedisClient, client)
37+
assert_kind_of(::RedisClient, client)
3838
assert_equal('master', client.call('ROLE').first)
3939
end
4040
end
4141

4242
def test_replica_clients
4343
got = @test_topology.replica_clients
4444
got.each_value do |client|
45-
assert_instance_of(::RedisClient, client)
45+
assert_kind_of(::RedisClient, client)
4646
assert_equal('slave', client.call('ROLE').first)
4747
end
4848
end
4949

5050
def test_clients_for_scanning
5151
got = @test_topology.clients_for_scanning
52-
got.each_value { |client| assert_instance_of(::RedisClient, client) }
52+
got.each_value { |client| assert_kind_of(::RedisClient, client) }
5353
assert_equal(TEST_SHARD_SIZE, got.size)
5454
end
5555

‎test/redis_client/cluster/node/test_random_replica_or_primary.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ class TestRandomReplicaWithPrimary < TestingWrapper
1111

1212
def test_clients_with_redis_client
1313
got = @test_topology.clients
14-
got.each_value { |client| assert_instance_of(::RedisClient, client) }
14+
got.each_value { |client| assert_kind_of(::RedisClient, client) }
1515
assert_equal(%w[master slave], got.map { |_, v| v.call('ROLE').first }.uniq.sort)
1616
end
1717

@@ -25,7 +25,7 @@ def test_clients_with_pooled_redis_client
2525
)
2626

2727
got = test_topology.clients
28-
got.each_value { |client| assert_instance_of(::RedisClient::Pooled, client) }
28+
got.each_value { |client| assert_kind_of(::RedisClient::Pooled, client) }
2929
assert_equal(%w[master slave], got.map { |_, v| v.call('ROLE').first }.uniq.sort)
3030
ensure
3131
test_topology&.clients&.each_value(&:close)
@@ -34,22 +34,22 @@ def test_clients_with_pooled_redis_client
3434
def test_primary_clients
3535
got = @test_topology.primary_clients
3636
got.each_value do |client|
37-
assert_instance_of(::RedisClient, client)
37+
assert_kind_of(::RedisClient, client)
3838
assert_equal('master', client.call('ROLE').first)
3939
end
4040
end
4141

4242
def test_replica_clients
4343
got = @test_topology.replica_clients
4444
got.each_value do |client|
45-
assert_instance_of(::RedisClient, client)
45+
assert_kind_of(::RedisClient, client)
4646
assert_equal('slave', client.call('ROLE').first)
4747
end
4848
end
4949

5050
def test_clients_for_scanning
5151
got = @test_topology.clients_for_scanning
52-
got.each_value { |client| assert_instance_of(::RedisClient, client) }
52+
got.each_value { |client| assert_kind_of(::RedisClient, client) }
5353
assert_equal(TEST_SHARD_SIZE, got.size)
5454
end
5555

‎test/redis_client/cluster/test_node.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,14 +331,14 @@ def test_find_by
331331
msg = "Case: primary only: #{info.node_key}"
332332
got = -> { @test_node.find_by(info.node_key) }
333333
if info.primary?
334-
assert_instance_of(::RedisClient, got.call, msg)
334+
assert_kind_of(::RedisClient, got.call, msg)
335335
else
336336
assert_raises(::RedisClient::Cluster::Node::ReloadNeeded, msg, &got)
337337
end
338338

339339
msg = "Case: scale read: #{info.node_key}"
340340
got = @test_node_with_scale_read.find_by(info.node_key)
341-
assert_instance_of(::RedisClient, got, msg)
341+
assert_kind_of(::RedisClient, got, msg)
342342
end
343343
end
344344

0 commit comments

Comments
 (0)