Skip to content

Commit

Permalink
Ensure command details are populated for Cluster clients
Browse files Browse the repository at this point in the history
Previously, if a clustered client is unable to query any of the nodes
for command details during setup, it'd initialize successfully, but
without any command details. (This is a bit of a corner case - in order
to hit this case, at least one node needs to be reachable when querying
nodes + slots, but all nodes would need to be unreachable immediately
afterwards when querying command details.)
In this case, the client would be unable to determine which node a
given command should be routed to. Because the client falls back to
routing commands to a random node, and following redirects as
necessary, this wouldn't cause any overt problems, but leaves the client
following unnecessary redirects for the rest of the lifespan of the
client.

This patch changes the client to require that the initial fetch of
command details succeeds (as is already done with slot and node detail
fetching), and raises a CannotConnectError if that's not possible.
Users should already handle this error, and retry creating the client
later, or use some other suitable failure handling mechanism.
  • Loading branch information
praboud-stripe committed Sep 9, 2021
1 parent 688ac95 commit 4cda62b
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 9 deletions.
13 changes: 6 additions & 7 deletions lib/redis/cluster/command_loader.rb
Expand Up @@ -10,22 +10,21 @@ module CommandLoader
module_function

def load(nodes)
details = {}

nodes.each do |node|
details = fetch_command_details(node)
details.empty? ? next : break
begin
return fetch_command_details(node)
rescue CannotConnectError, ConnectionError, CommandError
next # can retry on another node
end
end

details
raise CannotConnectError, 'Redis client could not connect to any cluster nodes'
end

def fetch_command_details(node)
node.call(%i[command]).map do |reply|
[reply[0], { arity: reply[1], flags: reply[2], first: reply[3], last: reply[4], step: reply[5] }]
end.to_h
rescue CannotConnectError, ConnectionError, CommandError
{} # can retry on another node
end

private_class_method :fetch_command_details
Expand Down
15 changes: 13 additions & 2 deletions test/cluster_client_key_hash_tags_test.rb
Expand Up @@ -6,8 +6,8 @@
class TestClusterClientKeyHashTags < Minitest::Test
include Helper::Cluster

def build_described_class
option = Redis::Cluster::Option.new(cluster: ['redis://127.0.0.1:7000'])
def build_described_class(urls=['redis://127.0.0.1:7000'])
option = Redis::Cluster::Option.new(cluster: urls)
node = Redis::Cluster::Node.new(option.per_node_key)
details = Redis::Cluster::CommandLoader.load(node)
Redis::Cluster::Command.new(details)
Expand Down Expand Up @@ -85,4 +85,15 @@ def test_whether_the_command_effect_is_readonly_or_not
assert_equal false, described_class.should_send_to_slave?([:info])
end
end

def test_cannot_build_details_from_bad_urls
assert_raises(Redis::CannotConnectError) do
build_described_class(['redis://127.0.0.1:7006'])
end
end

def test_builds_details_from_a_mix_of_good_and_bad_urls
described_class = build_described_class(['redis://127.0.0.1:7006', 'redis://127.0.0.1:7000'])
assert_equal 'dogs:1', described_class.extract_first_key(%w[get dogs:1])
end
end

0 comments on commit 4cda62b

Please sign in to comment.