From c6a1d64531ecb1fef41d494dc9bf6ebdd397c489 Mon Sep 17 00:00:00 2001 From: Vinay Krishna Pudyodu Date: Fri, 27 Dec 2024 17:20:53 -0800 Subject: [PATCH 1/2] Disabled auto_expand_replicas setting when search replica is enabled Signed-off-by: Vinay Krishna Pudyodu --- .../metadata/MetadataCreateIndexService.java | 58 +++++++++++++ .../MetadataUpdateSettingsService.java | 6 ++ ...utoExpandReplicaForSearchReplicaTests.java | 84 +++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 server/src/test/java/org/opensearch/cluster/metadata/DisableAutoExpandReplicaForSearchReplicaTests.java diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 232201d18ba13..6812a53be64f7 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -1469,6 +1469,7 @@ public void validateIndexSettings(String indexName, final Settings settings, fin throws IndexCreationException { List validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings, indexName); validateIndexReplicationTypeSettings(settings, clusterService.getClusterSettings()).ifPresent(validationErrors::add); + validateAutoExpandReplicaConflictInRequest(settings).ifPresent(validationErrors::add); validateErrors(indexName, validationErrors); } @@ -1738,6 +1739,63 @@ public static int calculateNumRoutingShards(int numShards, Version indexVersionC return numShards * 1 << numSplits; } + /** + * Validates that the settings do not conflict with each other. + * + * @param requestSettings settings passed in during index create request + * @return the validation error if there is one, otherwise {@code Optional.empty()} + */ + public static Optional validateAutoExpandReplicaConflictInRequest(Settings requestSettings) { + boolean hasRequestAutoExpand = AutoExpandReplicas.SETTING.get(requestSettings).isEnabled(); + boolean hasRequestSearchReplica = INDEX_NUMBER_OF_SEARCH_REPLICAS_SETTING.get(requestSettings) > 0; + if (hasRequestAutoExpand && hasRequestSearchReplica) { + return Optional.of( + "Cannot set both [" + + SETTING_AUTO_EXPAND_REPLICAS + + "] and [" + + SETTING_NUMBER_OF_SEARCH_REPLICAS + + "]. These settings are mutually exclusive." + ); + } + return Optional.empty(); + } + + /** + * Validates that the settings do not conflict with each other. + * Check conflicts between request and existing settings + * @param requestSettings + * @param indexSettings + * @return + */ + public static Optional validateAutoExpandReplicaConflictWithIndex(Settings requestSettings, Settings indexSettings) { + boolean hasRequestAutoExpand = AutoExpandReplicas.SETTING.get(requestSettings).isEnabled(); + boolean hasRequestSearchReplica = INDEX_NUMBER_OF_SEARCH_REPLICAS_SETTING.get(requestSettings) > 0; + boolean hasIndexAutoExpand = AutoExpandReplicas.SETTING.get(indexSettings).isEnabled(); + boolean hasIndexSearchReplica = INDEX_NUMBER_OF_SEARCH_REPLICAS_SETTING.get(indexSettings) > 0; + + if (hasRequestAutoExpand && hasIndexSearchReplica) { + return Optional.of( + "Cannot set [" + + AutoExpandReplicas.SETTING.getKey() + + "] because [" + + INDEX_NUMBER_OF_SEARCH_REPLICAS_SETTING.getKey() + + "] is set" + + ". These settings are mutually exclusive." + ); + } + + if (hasRequestSearchReplica && hasIndexAutoExpand) { + return Optional.of( + "Cannot set [" + + INDEX_NUMBER_OF_SEARCH_REPLICAS_SETTING.getKey() + + "] because [" + + AutoExpandReplicas.SETTING.getKey() + + "] is configured. These settings are mutually exclusive." + ); + } + return Optional.empty(); + } + public static void validateTranslogRetentionSettings(Settings indexSettings) { if (IndexSettings.INDEX_SOFT_DELETES_SETTING.get(indexSettings)) { if (IndexSettings.INDEX_TRANSLOG_RETENTION_AGE_SETTING.exists(indexSettings) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java index 8c350d6b9cef5..f2753c6d7994c 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -79,6 +79,8 @@ import static org.opensearch.action.support.ContextPreservingActionListener.wrapPreservingContext; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REPLICATION_TYPE_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS; +import static org.opensearch.cluster.metadata.MetadataCreateIndexService.validateAutoExpandReplicaConflictInRequest; +import static org.opensearch.cluster.metadata.MetadataCreateIndexService.validateAutoExpandReplicaConflictWithIndex; import static org.opensearch.cluster.metadata.MetadataCreateIndexService.validateOverlap; import static org.opensearch.cluster.metadata.MetadataCreateIndexService.validateRefreshIntervalSettings; import static org.opensearch.cluster.metadata.MetadataCreateIndexService.validateTranslogDurabilitySettings; @@ -229,6 +231,10 @@ public ClusterState execute(ClusterState currentState) { metadata.getSettings() ).ifPresent(validationErrors::add); + validateAutoExpandReplicaConflictInRequest(normalizedSettings).ifPresent(validationErrors::add); + validateAutoExpandReplicaConflictWithIndex(normalizedSettings, metadata.getSettings()).ifPresent( + validationErrors::add + ); } if (validationErrors.size() > 0) { diff --git a/server/src/test/java/org/opensearch/cluster/metadata/DisableAutoExpandReplicaForSearchReplicaTests.java b/server/src/test/java/org/opensearch/cluster/metadata/DisableAutoExpandReplicaForSearchReplicaTests.java new file mode 100644 index 0000000000000..6677f66552c05 --- /dev/null +++ b/server/src/test/java/org/opensearch/cluster/metadata/DisableAutoExpandReplicaForSearchReplicaTests.java @@ -0,0 +1,84 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.cluster.metadata; + +import org.opensearch.common.settings.Settings; +import org.opensearch.test.OpenSearchTestCase; + +/** + * AutoExpandReplica Disabled for Search Replica + */ +public class DisableAutoExpandReplicaForSearchReplicaTests extends OpenSearchTestCase { + + public void testWhenAutoExpandReplicaAndSearchReplicaIsSetInTheRequest() { + Settings conflictRequestSettings = Settings.builder() + .put("index.number_of_search_only_replicas", 1) + .put("index.auto_expand_replicas", "0-all") + .build(); + + assertEquals( + "Cannot set both [index.auto_expand_replicas] and [index.number_of_search_only_replicas]. These settings are mutually exclusive.", + MetadataCreateIndexService.validateAutoExpandReplicaConflictInRequest(conflictRequestSettings).get() + ); + } + + public void testWhenOnlyAutoExpandReplicaIsSetInTheRequest() { + Settings reqestSettings = Settings.builder() + .put("index.number_of_search_only_replicas", 0) + .put("index.auto_expand_replicas", "0-all") + .build(); + + assertTrue(MetadataCreateIndexService.validateAutoExpandReplicaConflictInRequest(reqestSettings).isEmpty()); + } + + public void testWhenAutoExpandReplicaEnabledForTheIndex() { + Settings reqestSettings = Settings.builder().put("index.number_of_search_only_replicas", 1).build(); + + Settings indexSettings = Settings.builder().put("index.auto_expand_replicas", "0-all").build(); + + assertEquals( + "Cannot set [index.number_of_search_only_replicas] because [index.auto_expand_replicas] is configured. These settings are mutually exclusive.", + MetadataCreateIndexService.validateAutoExpandReplicaConflictWithIndex(reqestSettings, indexSettings).get() + ); + } + + public void testWhenSearchReplicaEnabledForTheIndex() { + Settings reqestSettings = Settings.builder().put("index.auto_expand_replicas", "0-all").build(); + + Settings indexSettings = Settings.builder().put("index.number_of_search_only_replicas", 1).build(); + + assertEquals( + "Cannot set [index.auto_expand_replicas] because [index.number_of_search_only_replicas] is set. These settings are mutually exclusive.", + MetadataCreateIndexService.validateAutoExpandReplicaConflictWithIndex(reqestSettings, indexSettings).get() + ); + } + + public void testWhenSearchReplicaSetZeroForTheIndex() { + Settings reqestSettings = Settings.builder().put("index.auto_expand_replicas", "0-all").build(); + + Settings indexSettings = Settings.builder().put("index.number_of_search_only_replicas", 0).build(); + + assertTrue(MetadataCreateIndexService.validateAutoExpandReplicaConflictWithIndex(reqestSettings, indexSettings).isEmpty()); + } + + public void testIfSearchReplicaEnabledEnablingAutoExpandReplicasAndDisablingSearchReplicasNotPossibleInSingleRequest() { + Settings reqestSettings = Settings.builder() + .put("index.auto_expand_replicas", "0-all") + .put("index.number_of_search_only_replicas", 0) + .build(); + + Settings indexSettings = Settings.builder().put("index.number_of_search_only_replicas", 1).build(); + + assertEquals( + "Cannot set [index.auto_expand_replicas] because [index.number_of_search_only_replicas] " + + "is set. These settings are mutually exclusive.", + MetadataCreateIndexService.validateAutoExpandReplicaConflictWithIndex(reqestSettings, indexSettings).get() + ); + } +} From 52f6ca8bf7f6b937d570747b190045b7d943a4e1 Mon Sep 17 00:00:00 2001 From: Vinay Krishna Pudyodu Date: Thu, 2 Jan 2025 12:47:42 -0800 Subject: [PATCH 2/2] Added integration test Signed-off-by: Vinay Krishna Pudyodu --- CHANGELOG.md | 1 + .../AutoExpandWithSearchReplicaIT.java | 113 ++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 server/src/internalClusterTest/java/org/opensearch/cluster/metadata/AutoExpandWithSearchReplicaIT.java diff --git a/CHANGELOG.md b/CHANGELOG.md index a7f99722dd584..ec19de3870676 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Introduce a setting to disable download of full cluster state from remote on term mismatch([#16798](https://github.com/opensearch-project/OpenSearch/pull/16798/)) - Added ability to retrieve value from DocValues in a flat_object filed([#16802](https://github.com/opensearch-project/OpenSearch/pull/16802)) - Introduce framework for auxiliary transports and an experimental gRPC transport plugin ([#16534](https://github.com/opensearch-project/OpenSearch/pull/16534)) +- Disabled auto_expand_replicas setting when search replica is enabled ([#16916](https://github.com/opensearch-project/OpenSearch/pull/16916)) ### Dependencies - Bump `com.google.cloud:google-cloud-core-http` from 2.23.0 to 2.47.0 ([#16504](https://github.com/opensearch-project/OpenSearch/pull/16504)) diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/metadata/AutoExpandWithSearchReplicaIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/metadata/AutoExpandWithSearchReplicaIT.java new file mode 100644 index 0000000000000..001a7429a9f6b --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/metadata/AutoExpandWithSearchReplicaIT.java @@ -0,0 +1,113 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.cluster.metadata; + +import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; +import org.opensearch.indices.replication.common.ReplicationType; +import org.opensearch.test.OpenSearchIntegTestCase; + +public class AutoExpandWithSearchReplicaIT extends OpenSearchIntegTestCase { + + private static final String INDEX_NAME = "test-idx-1"; + + @Override + protected Settings featureFlagSettings() { + return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.READER_WRITER_SPLIT_EXPERIMENTAL, true).build(); + } + + public void testEnableAutoExpandWhenSearchReplicaActive() { + internalCluster().startDataOnlyNodes(3); + createIndex( + INDEX_NAME, + Settings.builder() + .put("number_of_shards", 1) + .put("number_of_replicas", 1) + .put("number_of_search_only_replicas", 1) + .put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT) + .build() + ); + ensureGreen(INDEX_NAME); + + expectThrows(IllegalArgumentException.class, () -> { + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.auto_expand_replicas", "0-1")) + .execute() + .actionGet(); + }); + + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.number_of_search_only_replicas", "0")) + .execute() + .actionGet(); + + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.auto_expand_replicas", "0-1")) + .execute() + .actionGet(); + + GetSettingsResponse response = client().admin().indices().prepareGetSettings(INDEX_NAME).execute().actionGet(); + + assertEquals("0-1", response.getSetting(INDEX_NAME, "index.auto_expand_replicas")); + } + + public void testEnableSearchReplicaWithAutoExpandActive() { + internalCluster().startDataOnlyNodes(3); + createIndex( + INDEX_NAME, + Settings.builder() + .put("number_of_shards", 1) + .put("number_of_replicas", 1) + .put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT) + .build() + ); + ensureGreen(INDEX_NAME); + + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.auto_expand_replicas", "0-1")) + .execute() + .actionGet(); + + expectThrows(IllegalArgumentException.class, () -> { + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.number_of_search_only_replicas", "1")) + .execute() + .actionGet(); + }); + + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.auto_expand_replicas", false)) + .execute() + .actionGet(); + + client().admin() + .indices() + .prepareUpdateSettings(INDEX_NAME) + .setSettings(Settings.builder().put("index.number_of_search_only_replicas", "1")) + .execute() + .actionGet(); + + GetSettingsResponse response = client().admin().indices().prepareGetSettings(INDEX_NAME).execute().actionGet(); + + assertEquals("1", response.getSetting(INDEX_NAME, "index.number_of_search_only_replicas")); + } +}