Code review changes

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
grafana · Jan 22, 2025 · 8e22c61 · 8e22c61
1 parent bf94082
commit 8e22c61
Show file tree

Hide file tree

Showing 2 changed files with 8 additions and 9 deletions.
diff --git a/ring/ring.go b/ring/ring.go
@@ -528,7 +528,6 @@ func (r *Ring) findInstancesForKey(key uint32, op Operation, bufDescs []Instance
 		// to have low single-digit number of hosts.
 		distinctHosts = bufHosts[:0]
 
-		// TODO: Do we need to pass this in to avoid allocations?
 		hostsPerZone       = make(map[string]int)
 		targetHostsPerZone = max(1, replicationFactor/maxZones)
 	)

diff --git a/ring/ring_test.go b/ring/ring_test.go
@@ -618,7 +618,7 @@ func TestRing_Get_ZoneAwareness(t *testing.T) {
 
 	tests := map[string]struct {
 		numInstances         int
-		numZones             int
+		zonesWithInstances   int
 		totalZones           int
 		replicationFactor    int
 		zoneAwarenessEnabled bool
@@ -627,32 +627,32 @@ func TestRing_Get_ZoneAwareness(t *testing.T) {
 	}{
 		"should succeed if there are enough instances per zone on RF = 3": {
 			numInstances:         16,
-			numZones:             3,
+			zonesWithInstances:   3,
 			totalZones:           3,
 			replicationFactor:    3,
 			zoneAwarenessEnabled: true,
 			expectedInstances:    3,
 		},
 		"should fail if there are instances in 1 zone only on RF = 3": {
 			numInstances:         16,
-			numZones:             1,
+			zonesWithInstances:   1,
 			totalZones:           3,
 			replicationFactor:    3,
 			zoneAwarenessEnabled: true,
 			expectedErr:          "at least 2 live replicas required across different availability zones, could only find 1",
 		},
 		"should succeed if there are instances in 2 zones on RF = 3": {
 			numInstances:         16,
-			numZones:             2,
+			zonesWithInstances:   2,
 			totalZones:           3,
 			replicationFactor:    3,
 			zoneAwarenessEnabled: true,
 			expectedInstances:    2,
 		},
 		"should succeed if there are instances in 1 zone only on RF = 3 but zone-awareness is disabled": {
 			numInstances:         16,
-			numZones:             1,
-			totalZones:           3,
+			zonesWithInstances:   1,
+			totalZones:           1,
 			replicationFactor:    3,
 			zoneAwarenessEnabled: false,
 			expectedInstances:    3,
@@ -670,15 +670,15 @@ func TestRing_Get_ZoneAwareness(t *testing.T) {
 				name := fmt.Sprintf("ing%v", i)
 				ingTokens := gen.GenerateTokens(128, prevTokens)
 
-				r.AddIngester(name, fmt.Sprintf("127.0.0.%d", i), fmt.Sprintf("zone-%v", i%testData.numZones), ingTokens, ACTIVE, time.Now(), false, time.Time{})
+				r.AddIngester(name, fmt.Sprintf("127.0.0.%d", i), fmt.Sprintf("zone-%v", i%testData.zonesWithInstances), ingTokens, ACTIVE, time.Now(), false, time.Time{})
 
 				prevTokens = append(prevTokens, ingTokens...)
 			}
 
 			// Add instances to the ring that don't own any tokens so that the ring is aware of all zones.
 			for i := testData.numInstances; i < testData.numInstances+testData.totalZones; i++ {
 				name := fmt.Sprintf("ing%v", i)
-				r.AddIngester(name, fmt.Sprintf("127.0.0.%d", i), fmt.Sprintf("zone-%v", i), nil, ACTIVE, time.Now(), false, time.Time{})
+				r.AddIngester(name, fmt.Sprintf("127.0.0.%d", i), fmt.Sprintf("zone-%v", i%testData.totalZones), nil, ACTIVE, time.Now(), false, time.Time{})
 			}
 
 			// Create a ring with the instances