Merge pull request #1126 from fatkodima/redis_distributed-change-hashing

Use `MD5` for hashing server nodes in `Redis::Distributed`

Author: byroot

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 # Unreleased 5.0.0
 
+- Use `MD5` for hashing server nodes in `Redis::Distributed`. This should improve keys distribution among servers. See #1089.
 - Cluster support has been moved to a `redis_cluster` companion gem.
 - `select` no longer record the current database. If the client has to reconnect after `select` was used, it will reconnect to the original database.
 - Removed `logger` option.

lib/redis/hash_ring.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'zlib'
+require 'digest/md5'
 
 class Redis
   class HashRing
@@ -25,7 +26,7 @@ def initialize(nodes = [], replicas = POINTS_PER_SERVER)
     def add_node(node)
       @nodes << node
       @replicas.times do |i|
-        key = Zlib.crc32("#{node.id}:#{i}")
+        key = server_hash_for("#{node.id}:#{i}")
         @ring[key] = node
         @sorted_keys << key
       end
@@ -35,7 +36,7 @@ def add_node(node)
     def remove_node(node)
       @nodes.reject! { |n| n.id == node.id }
       @replicas.times do |i|
-        key = Zlib.crc32("#{node.id}:#{i}")
+        key = server_hash_for("#{node.id}:#{i}")
         @ring.delete(key)
         @sorted_keys.reject! { |k| k == key }
       end
@@ -49,8 +50,8 @@ def get_node(key)
     def get_node_pos(key)
       return [nil, nil] if @ring.empty?
 
-      crc = Zlib.crc32(key)
-      idx = HashRing.binary_search(@sorted_keys, crc)
+      hash = hash_for(key)
+      idx = HashRing.binary_search(@sorted_keys, hash)
       [@ring[@sorted_keys[idx]], idx]
     end
 
@@ -85,5 +86,15 @@ def self.binary_search(ary, value)
       upper = ary.size - 1 if upper < 0
       upper
     end
+
+    private
+
+    def hash_for(key)
+      Zlib.crc32(key)
+    end
+
+    def server_hash_for(key)
+      Digest::MD5.digest(key).unpack1("L>")
+    end
   end
 end

test/distributed/key_tags_test.rb

@@ -28,7 +28,7 @@ def test_allows_clustering_of_keys
   end
 
   def test_distributes_keys_if_no_clustering_is_used
-    r.add_node("redis://127.0.0.1:#{PORT}/14")
+    r.add_node("redis://127.0.0.1:#{PORT}/13")
     r.flushdb
 
     r.set "users:1", 1