Address review comments

elastic · Jun 13, 2022 · ec7d7f9 · ec7d7f9
1 parent 8a1474d
commit ec7d7f9
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 22 deletions.
diff --git a/...src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java b/...src/internalClusterTest/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasIT.java
@@ -58,7 +58,22 @@ public void testClampToMinMax() throws Exception {
                 equalTo("0")
             );
         });
+        ensureGreen(indexName);
+
+        // Remove the setting
+        updateIndexSettings(indexName, Settings.builder().put("index.routing.allocation.require._id", ""));
 
+        assertBusy(() -> {
+            assertThat(
+                client().admin()
+                    .indices()
+                    .prepareGetSettings(indexName)
+                    .setNames("index.number_of_replicas")
+                    .get()
+                    .getSetting(indexName, "index.number_of_replicas"),
+                equalTo(String.valueOf(initialReplicas))
+            );
+        });
         ensureGreen(indexName);
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java b/server/src/main/java/org/elasticsearch/cluster/metadata/AutoExpandReplicas.java
@@ -92,10 +92,6 @@ private static AutoExpandReplicas parse(String value) {
         }
     }
 
-    int getMaxReplicas(int numDataNodes) {
-        return Math.min(maxReplicas, numDataNodes - 1);
-    }
-
     public boolean expandToAllNodes() {
         return maxReplicas == Integer.MAX_VALUE;
     }
@@ -114,14 +110,12 @@ public int getDesiredNumberOfReplicas(IndexMetadata indexMetadata, RoutingAlloca
 
     // package private for testing
     int calculateDesiredNumberOfReplicas(int numMatchingDataNodes) {
-        final int min = minReplicas();
-        final int max = getMaxReplicas(numMatchingDataNodes);
         int numberOfReplicas = numMatchingDataNodes - 1;
         // Make sure number of replicas is always between min and max
-        if (numberOfReplicas < min) {
-            numberOfReplicas = min;
-        } else if (numberOfReplicas > max) {
-            numberOfReplicas = max;
+        if (numberOfReplicas < minReplicas) {
+            numberOfReplicas = minReplicas;
+        } else if (numberOfReplicas > maxReplicas) {
+            numberOfReplicas = maxReplicas;
         }
         return numberOfReplicas;
     }

diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java
@@ -50,22 +50,18 @@ public void testParseSettings() {
             Settings.builder().put("index.auto_expand_replicas", "0-5").build()
         );
         assertEquals(0, autoExpandReplicas.minReplicas());
-        assertEquals(5, autoExpandReplicas.getMaxReplicas(8));
-        assertEquals(2, autoExpandReplicas.getMaxReplicas(3));
+        assertEquals(5, autoExpandReplicas.maxReplicas());
         assertFalse(autoExpandReplicas.expandToAllNodes());
 
         autoExpandReplicas = AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "0-all").build());
         assertEquals(0, autoExpandReplicas.minReplicas());
-        assertEquals(5, autoExpandReplicas.getMaxReplicas(6));
-        assertEquals(2, autoExpandReplicas.getMaxReplicas(3));
+        assertEquals(Integer.MAX_VALUE, autoExpandReplicas.maxReplicas());
         assertTrue(autoExpandReplicas.expandToAllNodes());
 
         autoExpandReplicas = AutoExpandReplicas.SETTING.get(Settings.builder().put("index.auto_expand_replicas", "1-all").build());
         assertEquals(1, autoExpandReplicas.minReplicas());
-        assertEquals(5, autoExpandReplicas.getMaxReplicas(6));
-        assertEquals(2, autoExpandReplicas.getMaxReplicas(3));
+        assertEquals(Integer.MAX_VALUE, autoExpandReplicas.maxReplicas());
         assertTrue(autoExpandReplicas.expandToAllNodes());
-
     }
 
     public void testInvalidValues() {
@@ -284,14 +280,15 @@ public void testOnlyAutoExpandAllocationFilteringAfterAllNodesUpgraded() {
     }
 
     public void testCalculateDesiredNumberOfReplicas() {
-        String settingValue = randomFrom("0-all", "0-3");
+        int lowerBound = between(0, 9);
+        int upperBound = between(lowerBound + 1, 10);
+        String settingValue = lowerBound + "-" + randomFrom(upperBound, "all");
         AutoExpandReplicas autoExpandReplicas = AutoExpandReplicas.SETTING.get(
             Settings.builder().put(SETTING_AUTO_EXPAND_REPLICAS, settingValue).build()
         );
         int max = autoExpandReplicas.maxReplicas();
-        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(0), equalTo(0));
-        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(1), equalTo(0));
-        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(2), equalTo(1));
-        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(max), equalTo(max - 1));
+        int matchingNodes = between(0, max);
+        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(matchingNodes), equalTo(Math.max(lowerBound, matchingNodes - 1)));
+        assertThat(autoExpandReplicas.calculateDesiredNumberOfReplicas(max + 1), equalTo(max));
     }
 }