fix: a bug to parse reply of cluster nodes command (#69)

Author: supercaracal

.github/workflows/test.yaml

@@ -158,3 +158,34 @@ jobs:
         run: bundle exec rake test_cluster_broken
       - name: Stop containers
         run: docker compose -f $DOCKER_COMPOSE_FILE down
+  cluster-scale:
+    name: Cluster Scale
+    timeout-minutes: 15
+    strategy:
+      fail-fast: false
+      matrix:
+        redis: ['7.0.1', '6.2.7']
+    runs-on: ubuntu-latest
+    env:
+      REDIS_VERSION: ${{ matrix.redis }}
+      DOCKER_COMPOSE_FILE: 'docker-compose.scale.yaml'
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v3
+      - name: Set up Ruby
+        uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: '3.1'
+          bundler-cache: true
+      - name: Pull Docker images
+        run: docker pull redis:$REDIS_VERSION
+      - name: Run containers
+        run: docker compose -f $DOCKER_COMPOSE_FILE up -d
+      - name: Wait for Redis cluster to be ready
+        run: bundle exec rake wait
+      - name: Print containers
+        run: docker compose -f $DOCKER_COMPOSE_FILE ps
+      - name: Run minitest
+        run: bundle exec rake test_cluster_scale
+      - name: Stop containers
+        run: docker compose -f $DOCKER_COMPOSE_FILE down

Rakefile

@@ -2,29 +2,28 @@
 
 require 'rake/testtask'
 
+FREAKY_TEST_TYPES = %w[broken scale state].freeze
+
 task default: :test
 
-Rake::TestTask.new :test do |t|
-  t.libs << :test
+Rake::TestTask.new(:test) do |t|
   t.libs << :lib
-  files = Dir['test/**/test_*.rb'].grep_v(/test_against_cluster_(state|broken)/)
-  files = ARGV[1..] if ARGV.size > 1
-  t.test_files = files
-  t.options = '-v'
-end
-
-Rake::TestTask.new :test_cluster_state do |t|
   t.libs << :test
-  t.libs << :lib
-  t.test_files = %w[test/test_against_cluster_state.rb]
   t.options = '-v'
+  t.test_files = if ARGV.size > 1
+                   ARGV[1..]
+                 else
+                   Dir['test/**/test_*.rb'].grep_v(/test_against_cluster_(#{FREAKY_TEST_TYPES.join('|')})/)
+                 end
 end
 
-Rake::TestTask.new :test_cluster_broken do |t|
-  t.libs << :test
-  t.libs << :lib
-  t.test_files = %w[test/test_against_cluster_broken.rb]
-  t.options = '-v'
+FREAKY_TEST_TYPES.each do |type|
+  Rake::TestTask.new("test_cluster_#{type}".to_sym) do |t|
+    t.libs << :lib
+    t.libs << :test
+    t.options = '-v'
+    t.test_files = ["test/test_against_cluster_#{type}.rb"]
+  end
 end
 
 desc 'Wait for cluster to be ready'

docker-compose.scale.yaml

@@ -0,0 +1,79 @@
+---
+services:
+  node1: &node
+    image: "redis:${REDIS_VERSION:-7}"
+    command: >
+      redis-server
+      --maxmemory            64mb
+      --maxmemory-policy     allkeys-lru
+      --appendonly           yes
+      --cluster-enabled      yes
+      --cluster-config-file  nodes.conf
+      --cluster-node-timeout 5000
+    restart: "${RESTART_POLICY:-always}"
+    healthcheck:
+      test: ["CMD", "redis-cli", "ping"]
+      interval: "7s"
+      timeout: "5s"
+      retries: 10
+    ports:
+      - "6379:6379"
+  node2:
+    <<: *node
+    ports:
+      - "6380:6379"
+  node3:
+    <<: *node
+    ports:
+      - "6381:6379"
+  node4:
+    <<: *node
+    ports:
+      - "6382:6379"
+  node5:
+    <<: *node
+    ports:
+      - "6383:6379"
+  node6:
+    <<: *node
+    ports:
+      - "6384:6379"
+  node7:
+    <<: *node
+    ports:
+      - "6385:6379"
+  node8:
+    <<: *node
+    ports:
+      - "6386:6379"
+  clustering:
+    image: "redis:${REDIS_VERSION:-7}"
+    command: >
+      bash -c "apt-get update > /dev/null
+      && apt-get install --no-install-recommends --no-install-suggests -y dnsutils > /dev/null
+      && rm -rf /var/lib/apt/lists/*
+      && yes yes | redis-cli --cluster create
+      $$(dig node1 +short):6379
+      $$(dig node2 +short):6379
+      $$(dig node3 +short):6379
+      $$(dig node4 +short):6379
+      $$(dig node5 +short):6379
+      $$(dig node6 +short):6379
+      --cluster-replicas 1"
+    depends_on:
+      node1:
+        condition: service_healthy
+      node2:
+        condition: service_healthy
+      node3:
+        condition: service_healthy
+      node4:
+        condition: service_healthy
+      node5:
+        condition: service_healthy
+      node6:
+        condition: service_healthy
+      node7:
+        condition: service_healthy
+      node8:
+        condition: service_healthy

lib/redis_client/cluster/command.rb

@@ -81,7 +81,7 @@ def determine_first_key_position(command) # rubocop:disable Metrics/CyclomaticCo
         when 'memory'
           command[1].to_s.casecmp('usage').zero? ? 2 : 0
         when 'migrate'
-          command[3] == '""' ? determine_optional_key_position(command, 'keys') : 3
+          command[3].empty? ? determine_optional_key_position(command, 'keys') : 3
         when 'xread', 'xreadgroup'
           determine_optional_key_position(command, 'streams')
         else

lib/redis_client/cluster/errors.rb

@@ -16,9 +16,9 @@ def initialize(errors)
     class OrchestrationCommandNotSupported < ::RedisClient::Error
       def initialize(command)
         str = ERR_ARG_NORMALIZATION.call(command).map(&:to_s).join(' ').upcase
-        msg = "#{str} command should be used with care "\
-              'only by applications orchestrating Redis Cluster, like redis-cli, '\
-              'and the command if used out of the right context can leave the cluster '\
+        msg = "#{str} command should be used with care " \
+              'only by applications orchestrating Redis Cluster, like redis-cli, ' \
+              'and the command if used out of the right context can leave the cluster ' \
               'in a wrong state or cause data loss.'
         super(msg)
       end
@@ -49,8 +49,8 @@ def initialize(command)
     class NodeMightBeDown < ::RedisClient::Error
       def initialize(_ = '')
         super(
-          'The client is trying to fetch the latest cluster state '\
-          'because a subset of nodes might be down. '\
+          'The client is trying to fetch the latest cluster state ' \
+          'because a subset of nodes might be down. ' \
           'It might continue to raise errors for a while.'
         )
       end

lib/redis_client/cluster/node.rb

@@ -77,9 +77,8 @@ def parse_node_info(info) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticC
               arr[8] = []
               next
             end
-
-            arr[8] = arr[8].split(',').map { |r| r.split('-').map { |s| Integer(s) } }
-            arr[8] = arr[8].map { |a| a.size == 1 ? a << a.first : a }.map(&:sort)
+            arr[8] = arr[8..].filter_map { |str| str.start_with?('[') ? nil : str.split('-').map { |s| Integer(s) } }
+                             .map { |a| a.size == 1 ? a << a.first : a }.map(&:sort)
           end
 
           rows.map do |arr|