From df4bdbf688c2e4f5d9305d6d59ec3086ab701d04 Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Fri, 21 Mar 2014 13:09:23 -0400 Subject: [PATCH] Cluster: Fix trib create when masters==replicas This bug was introduced in 2e5c394f during a refactor. It took me a while to understand what was going on with the code, so I've refactored it further by: - Replacing boolean values with meaningful symbols - Replacing 'i' with a meaningful variable name - Adding the proper abort check - Factoring out now duplicated conditionals - Adding optional verbose logging (we're inside *four* different looping constructs, so it takes a while to figure out where all the moving parts are) - Updating comment for the section This fixes a problem when the number of master instances equaled the number of replica instances. Before, when there were equal numbers of both, nodes_count would go to zero, but the while loop would spin in i < @replicas because i would never be updated (because the nodes_list of each ip was length == 0, which triggered an endless loop of next -> i = 0 -> 0 < 1? -> true -> next -> i = 0 ...) Thanks to carlo who found this problem at: https://groups.google.com/forum/#!topic/redis-db/_WVVqDw5B7c --- src/redis-trib.rb | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/redis-trib.rb b/src/redis-trib.rb index cfe04d20..4efe11cb 100755 --- a/src/redis-trib.rb +++ b/src/redis-trib.rb @@ -554,14 +554,28 @@ class RedisTrib # We try to split the replicas among all the IPs with spare nodes # trying to avoid the host where the master is running, if possible. # - # Note that we loop two times, the first with spare_loop set to false, - # the second with the var set to true. The second loop changes the - # while condition so to assign remaining slaves. - [false,true].each{|spare_loop| + # Note we loop two times. The first loop assigns the requested + # number of replicas to each master. The second loop assigns any + # remaining instances as extra replicas to masters. Some masters + # may end up with more than their requested number of replicas, but + # all nodes will be used. + assignment_verbose = false + + [:requested,:unused].each{|assign| masters.each{|m| - i = 0 - while (!spare_loop && i < @replicas) || \ - (spare_loop && nodes_count > 0) + assigned_replicas = 0 + while assigned_replicas < @replicas + break if nodes_count == 0 + if assignment_verbose + if assign == :requested + puts "Requesting total of #{@replicas} replicas " \ + "(#{assigned_replicas} replicas assigned " \ + "so far with #{nodes_count} total remaining)." + elsif assign == :unused + puts "Assigning extra instance to replication " \ + "role too (#{nodes_count} remaining)." + end + end ips.each{|ip,nodes_list| next if nodes_list.length == 0 # Skip instances with the same IP as the master if we @@ -570,7 +584,7 @@ class RedisTrib slave = nodes_list.shift slave.set_as_replica(m.info[:name]) nodes_count -= 1 - i += 1 + assigned_replicas += 1 puts "Adding replica #{slave} to #{m}" } end