feat: introduce max_startup_sample in ClusterConfig (#369)

Author: supercaracal

README.md

@@ -30,6 +30,7 @@ gem 'redis-cluster-client'
 | `:slow_command_timeout` | Integer | `-1` | timeout used for "slow" queries that fetch metdata e.g. CLUSTER NODES, COMMAND |
 | `:concurrency` | Hash | `{ model: :on_demand, size: 5}` | concurrency settings, `:on_demand`, `:pooled` and `:none` are valid models, size is a max number of workers, `:none` model is no concurrency, Please choose the one suited your environment if needed. |
 | `:connect_with_original_config` | Boolean | `false` | `true` if client should retry the connection using the original endpoint that was passed in |
+| `:max_startup_sample` | Integer | `3` | maximum number of nodes to fetch `CLUSTER NODES` information for startup |
 
 Also, [the other generic options](https://github.com/redis-rb/redis-client#configuration) can be passed.
 But `:url`, `:host`, `:port` and `:path` are ignored because they conflict with the `:nodes` option.

lib/redis_client/cluster/node.rb

@@ -13,9 +13,6 @@ class Cluster
     class Node
       include Enumerable
 
-      # It affects to strike a balance between load and stability in initialization or changed states.
-      MAX_STARTUP_SAMPLE = Integer(ENV.fetch('REDIS_CLIENT_MAX_STARTUP_SAMPLE', 3))
-
       # less memory consumption, but slow
       USE_CHAR_ARRAY_SLOT = Integer(ENV.fetch('REDIS_CLIENT_USE_CHAR_ARRAY_SLOT', 1)) == 1
 
@@ -197,7 +194,7 @@ def update_slot(slot, node_key)
 
       def reload!
         with_reload_lock do
-          with_startup_clients(MAX_STARTUP_SAMPLE) do |startup_clients|
+          with_startup_clients(@config.max_startup_sample) do |startup_clients|
             @node_info = refetch_node_info_list(startup_clients)
             @node_configs = @node_info.to_h do |node_info|
               [node_info.node_key, @config.client_config_for_node(node_info.node_key)]

lib/redis_client/cluster_config.rb

@@ -20,13 +20,15 @@ class ClusterConfig
     MAX_WORKERS = Integer(ENV.fetch('REDIS_CLIENT_MAX_THREADS', 5))
     # It's used with slow queries of fetching meta data like CLUSTER NODES, COMMAND and so on.
     SLOW_COMMAND_TIMEOUT = Float(ENV.fetch('REDIS_CLIENT_SLOW_COMMAND_TIMEOUT', -1))
+    # It affects to strike a balance between load and stability in initialization or changed states.
+    MAX_STARTUP_SAMPLE = Integer(ENV.fetch('REDIS_CLIENT_MAX_STARTUP_SAMPLE', 3))
 
     InvalidClientConfigError = Class.new(::RedisClient::Error)
 
     attr_reader :command_builder, :client_config, :replica_affinity, :slow_command_timeout,
-                :connect_with_original_config, :startup_nodes
+                :connect_with_original_config, :startup_nodes, :max_startup_sample
 
-    def initialize(
+    def initialize( # rubocop:disable Metrics/ParameterLists
       nodes: DEFAULT_NODES,
       replica: false,
       replica_affinity: :random,
@@ -36,6 +38,7 @@ def initialize(
       client_implementation: ::RedisClient::Cluster, # for redis gem
       slow_command_timeout: SLOW_COMMAND_TIMEOUT,
       command_builder: ::RedisClient::CommandBuilder,
+      max_startup_sample: MAX_STARTUP_SAMPLE,
       **client_config
     )
 
@@ -51,6 +54,7 @@ def initialize(
       @connect_with_original_config = connect_with_original_config
       @client_implementation = client_implementation
       @slow_command_timeout = slow_command_timeout
+      @max_startup_sample = max_startup_sample
     end
 
     def inspect

test/redis_client/cluster/test_node.rb

@@ -600,7 +600,7 @@ def test_reload
 
         # It should have reloaded by calling CLUSTER NODES on three of the startup nodes
         cluster_node_cmds = capture_buffer.to_a.select { |c| c.command == %w[CLUSTER NODES] }
-        assert_equal RedisClient::Cluster::Node::MAX_STARTUP_SAMPLE, cluster_node_cmds.size
+        assert_equal RedisClient::ClusterConfig::MAX_STARTUP_SAMPLE, cluster_node_cmds.size
 
         # It should have connected to all of the clients.
         assert_equal TEST_NUMBER_OF_NODES, test_node.to_a.size
@@ -635,6 +635,27 @@ def test_reload_with_original_config
         assert_equal bootstrap_node, cluster_node_cmds.first.server_url
       end
 
+      def test_reload_with_overriden_sample_size
+        capture_buffer = CommandCaptureMiddleware::CommandBuffer.new
+        test_node = make_node(replica: true, capture_buffer: capture_buffer, max_startup_sample: 1)
+
+        capture_buffer.clear
+        test_node.reload!
+
+        # It should have reloaded by calling CLUSTER NODES on one of the startup nodes
+        cluster_node_cmds = capture_buffer.to_a.select { |c| c.command == %w[CLUSTER NODES] }
+        assert_equal 1, cluster_node_cmds.size
+
+        # It should have connected to all of the clients.
+        assert_equal TEST_NUMBER_OF_NODES, test_node.to_a.size
+
+        # If we reload again, it should NOT change the redis client instances we have.
+        original_client_ids = test_node.to_a.map(&:object_id).to_set
+        test_node.reload!
+        new_client_ids = test_node.to_a.map(&:object_id).to_set
+        assert_equal original_client_ids, new_client_ids
+      end
+
       def test_reload_concurrently
         capture_buffer = CommandCaptureMiddleware::CommandBuffer.new
         test_node = make_node(replica: true, pool: { size: 2 }, capture_buffer: capture_buffer)
@@ -656,7 +677,7 @@ def refetch_node_info_list(...)
         # We should only have reloaded once, which is to say, we only called CLUSTER NODES command MAX_STARTUP_SAMPLE
         # times
         cluster_node_cmds = capture_buffer.to_a.select { |c| c.command == %w[CLUSTER NODES] }
-        assert_equal RedisClient::Cluster::Node::MAX_STARTUP_SAMPLE, cluster_node_cmds.size
+        assert_equal RedisClient::ClusterConfig::MAX_STARTUP_SAMPLE, cluster_node_cmds.size
       end
     end
     # rubocop:enable Metrics/ClassLength