code review feedback

client/client.go

@@ -2427,26 +2427,6 @@ DISCOLOOP:
 
 	consulLogger.Info("discovered following servers", "servers", nomadServers)
 
-	// Check if the list of servers discovered is identical to the list we already have
-	// If so, we don't need to reset the server list unnecessarily
-	knownServers := make(map[string]struct{})
-	serverList := c.servers.GetServers()
-	for _, s := range serverList {
-		knownServers[s.Addr.String()] = struct{}{}
-	}
-
-	allFound := true
-	for _, s := range nomadServers {
-		_, known := knownServers[s.Addr.String()]
-		if !known {
-			allFound = false
-			break
-		}
-	}
-	if allFound && len(nomadServers) == len(serverList) {
-		c.logger.Info("Not replacing server list, current server list is identical to servers discovered in Consul")
-		return nil
-	}
 	// Fire the retry trigger if we have updated the set of servers.
 	if c.servers.SetServers(nomadServers) {
 		// Start rebalancing

client/servers/manager.go

@@ -201,12 +201,16 @@ func (m *Manager) SetServers(servers Servers) bool {
 	m.Lock()
 	defer m.Unlock()
 
-	// Sort both the  existing and incoming servers
-	servers.Sort()
-	m.servers.Sort()
-
 	// Determine if they are equal
-	equal := servers.Equal(m.servers)
+	equal := m.serversAreEqual(servers)
+
+	// If server list is equal don't change the list and return immediatly
+	// This prevents unnecessary shuffling of a failed server that was moved to the
+	// bottom of the list
+	if equal {
+		m.logger.Debug("Not replacing server list, current server list is identical to servers discovered in Consul")
+		return !equal
+	}
 
 	// Randomize the incoming servers
 	servers.shuffle()
@@ -215,6 +219,23 @@ func (m *Manager) SetServers(servers Servers) bool {
 	return !equal
 }
 
+// Method to check if the arg list of servers is equal to the one we already have
+func (m *Manager) serversAreEqual(servers Servers) bool {
+	// We use a copy of the server list here because determining
+	// equality requires a sort step which modifies the order of the server list
+	var copy Servers
+	copy = make([]*Server, 0, len(m.servers))
+	for _, s := range m.servers {
+		copy = append(copy, s.Copy())
+	}
+
+	// Sort both the  existing and incoming servers
+	copy.Sort()
+	servers.Sort()
+
+	return copy.Equal(servers)
+}
+
 // FindServer returns a server to send an RPC too. If there are no servers, nil
 // is returned.
 func (m *Manager) FindServer() *Server {

client/servers/manager_test.go

@@ -66,6 +66,14 @@ func TestServers_SetServers(t *testing.T) {
 	require.True(m.SetServers([]*servers.Server{s1}))
 	require.Equal(1, m.NumServers())
 	require.Len(m.GetServers(), 1)
+
+	// Test that the list of servers does not get shuffled
+	// as a side effect when incoming list is equal
+	require.True(m.SetServers([]*servers.Server{s1, s2}))
+	before := m.GetServers()
+	require.False(m.SetServers([]*servers.Server{s1, s2}))
+	after := m.GetServers()
+	require.Equal(before, after)
 }
 
 func TestServers_FindServer(t *testing.T) {

dev/cluster/client3.hcl

@@ -7,6 +7,9 @@ data_dir = "/tmp/client3"
 # Give the agent a unique name. Defaults to hostname
 name = "client3"
 
+# Enable debugging
+enable_debug = true
+
 # Enable the client
 client {
   enabled = true

dev/cluster/server1.hcl

@@ -16,5 +16,5 @@ server {
   }
 
   # Self-elect, should be 3 or 5 for production
-  bootstrap_expect = 1
+  bootstrap_expect = 3
 }