Update setting API honors cluster's replica setting as default opense…

…arch-project#14810 (opensearch-project#14948) * Update setting API honors cluster's replica setting as default opensearch-project#14810 Signed-off-by: Liyun Xiu <xiliyun@amazon.com> * Update changelog & add one more test case Signed-off-by: Liyun Xiu <xiliyun@amazon.com> --------- Signed-off-by: Liyun Xiu <xiliyun@amazon.com> Signed-off-by: Daniel Widdis <widdis@gmail.com> Co-authored-by: Daniel Widdis <widdis@gmail.com>
dk2k · Oct 16, 2024 · 2faabe4 · 2faabe4
1 parent 14669f6
commit 2faabe4
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -67,6 +67,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix search_as_you_type not supporting multi-fields ([#15988](https://github.com/opensearch-project/OpenSearch/pull/15988))
 - Avoid infinite loop when `flat_object` field contains invalid token ([#15985](https://github.com/opensearch-project/OpenSearch/pull/15985))
 - Fix infinite loop in nested agg ([#15931](https://github.com/opensearch-project/OpenSearch/pull/15931))
+- Fix update settings with null replica not honoring cluster setting bug ([#14948](https://github.com/opensearch-project/OpenSearch/pull/14948))
 - Fix race condition in node-join and node-left ([#15521](https://github.com/opensearch-project/OpenSearch/pull/15521))
 - Streaming bulk request hangs ([#16158](https://github.com/opensearch-project/OpenSearch/pull/16158))
 - Fix warnings from SLF4J on startup when repository-s3 is installed ([#16194](https://github.com/opensearch-project/OpenSearch/pull/16194))

diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/UpdateSettingsIT.java
@@ -895,6 +895,69 @@ private void runTestDefaultNumberOfReplicasTest(final boolean closeIndex) {
         assertThat(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(response.getIndexToSettings().get("test")), equalTo(1));
     }
 
+    public void testNullReplicaUpdate() {
+        internalCluster().ensureAtLeastNumDataNodes(2);
+
+        // cluster setting
+        String defaultNumberOfReplica = "3";
+        assertAcked(
+            client().admin()
+                .cluster()
+                .prepareUpdateSettings()
+                .setPersistentSettings(Settings.builder().put("cluster.default_number_of_replicas", defaultNumberOfReplica))
+                .get()
+        );
+        // create index with number of replicas will ignore default value
+        assertAcked(
+            client().admin()
+                .indices()
+                .prepareCreate("test")
+                .setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, "2"))
+        );
+
+        String numberOfReplicas = client().admin()
+            .indices()
+            .prepareGetSettings("test")
+            .get()
+            .getSetting("test", IndexMetadata.SETTING_NUMBER_OF_REPLICAS);
+        assertEquals("2", numberOfReplicas);
+        // update setting with null replica will use cluster setting of replica number
+        assertAcked(
+            client().admin()
+                .indices()
+                .prepareUpdateSettings("test")
+                .setSettings(Settings.builder().putNull(IndexMetadata.SETTING_NUMBER_OF_REPLICAS))
+        );
+
+        numberOfReplicas = client().admin()
+            .indices()
+            .prepareGetSettings("test")
+            .get()
+            .getSetting("test", IndexMetadata.SETTING_NUMBER_OF_REPLICAS);
+        assertEquals(defaultNumberOfReplica, numberOfReplicas);
+
+        // Clean up cluster setting, then update setting with null replica should pick up default value of 1
+        assertAcked(
+            client().admin()
+                .cluster()
+                .prepareUpdateSettings()
+                .setPersistentSettings(Settings.builder().putNull("cluster.default_number_of_replicas"))
+        );
+        assertAcked(
+            client().admin()
+                .indices()
+                .prepareUpdateSettings("test")
+                .setSettings(Settings.builder().putNull(IndexMetadata.SETTING_NUMBER_OF_REPLICAS))
+        );
+
+        numberOfReplicas = client().admin()
+            .indices()
+            .prepareGetSettings("test")
+            .get()
+            .getSetting("test", IndexMetadata.SETTING_NUMBER_OF_REPLICAS);
+        assertEquals("1", numberOfReplicas);
+    }
+
     public void testNoopUpdate() {
         internalCluster().ensureAtLeastNumDataNodes(2);
         final ClusterService clusterService = internalCluster().getClusterManagerNodeInstance(ClusterService.class);

diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
@@ -140,6 +140,7 @@ public void updateSettings(
 
         validateRefreshIntervalSettings(normalizedSettings, clusterService.getClusterSettings());
         validateTranslogDurabilitySettings(normalizedSettings, clusterService.getClusterSettings(), clusterService.getSettings());
+        final int defaultReplicaCount = clusterService.getClusterSettings().get(Metadata.DEFAULT_REPLICA_COUNT_SETTING);
 
         Settings.Builder settingsForClosedIndices = Settings.builder();
         Settings.Builder settingsForOpenIndices = Settings.builder();
@@ -248,7 +249,10 @@ public ClusterState execute(ClusterState currentState) {
                     }
 
                     if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(openSettings)) {
-                        final int updatedNumberOfReplicas = IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(openSettings);
+                        final int updatedNumberOfReplicas = openSettings.getAsInt(
+                            IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
+                            defaultReplicaCount
+                        );
                         if (preserveExisting == false) {
                             for (Index index : request.indices()) {
                                 if (index.getName().charAt(0) != '.') {
@@ -329,15 +333,13 @@ public ClusterState execute(ClusterState currentState) {
                                 /*
                                  * The setting index.number_of_replicas is special; we require that this setting has a value in the index. When
                                  * creating the index, we ensure this by explicitly providing a value for the setting to the default (one) if
-                                 * there is a not value provided on the source of the index creation. A user can update this setting though,
-                                 * including updating it to null, indicating that they want to use the default value. In this case, we again
-                                 * have to provide an explicit value for the setting to the default (one).
+                                 * there is no value provided on the source of the index creation or "cluster.default_number_of_replicas" is
+                                 * not set. A user can update this setting though, including updating it to null, indicating that they want to
+                                 * use the value configured by cluster settings or a default value 1. In this case, we again have to provide
+                                 * an explicit value for the setting.
                                  */
                                 if (IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.exists(indexSettings) == false) {
-                                    indexSettings.put(
-                                        IndexMetadata.SETTING_NUMBER_OF_REPLICAS,
-                                        IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.get(Settings.EMPTY)
-                                    );
+                                    indexSettings.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, defaultReplicaCount);
                                 }
                                 Settings finalSettings = indexSettings.build();
                                 indexScopedSettings.validate(