diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexClusterDefaultDocRep.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexClusterDefaultDocRep.java index 2abf4fc50ec69..fbe821d049a17 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexClusterDefaultDocRep.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexClusterDefaultDocRep.java @@ -16,7 +16,11 @@ import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.test.OpenSearchIntegTestCase; +import java.util.Locale; + import static org.hamcrest.Matchers.containsString; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; import static org.opensearch.indices.IndicesService.CLUSTER_REPLICATION_TYPE_SETTING; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @@ -44,7 +48,15 @@ public void testDefaultRemoteStoreNoUserOverride() throws Exception { ); assertThat( exc.getMessage(), - containsString("Cannot enable [index.remote_store.enabled] when [index.replication.type] is DOCUMENT") + containsString( + String.format( + Locale.ROOT, + "To enable %s, %s should be set to %s", + SETTING_REMOTE_STORE_ENABLED, + SETTING_REPLICATION_TYPE, + ReplicationType.SEGMENT + ) + ) ); } diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java index e52a12f66cff4..2d2533500bf9d 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java @@ -26,14 +26,13 @@ import static org.hamcrest.Matchers.containsString; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; -import static org.opensearch.index.IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; +import static org.opensearch.index.IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING; import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_STORE_REPOSITORY_SETTING; import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING; import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_STORE_ENABLED_SETTING; import static org.opensearch.indices.IndicesService.CLUSTER_REPLICATION_TYPE_SETTING; -import static org.opensearch.indices.IndicesService.CLUSTER_SETTING_REPLICATION_TYPE; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST) @@ -111,20 +110,12 @@ public void testRemoteStoreDisabledByUser() throws Exception { .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) .put(SETTING_REMOTE_STORE_ENABLED, false) .build(); - assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); - GetIndexResponse getIndexResponse = client().admin() - .indices() - .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) - .get(); - Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); - verifyRemoteStoreIndexSettings( - indexSettings, - "false", - null, - null, - client().settings().get(CLUSTER_SETTING_REPLICATION_TYPE), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testRemoteStoreEnabledByUserWithoutRemoteRepoAndSegmentReplicationIllegalArgumentException() throws Exception { @@ -156,16 +147,7 @@ public void testRemoteStoreEnabledByUserWithoutRemoteRepoIllegalArgumentExceptio IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); - assertThat( - exc.getMessage(), - containsString( - String.format( - Locale.ROOT, - "Setting %s should be provided with non-empty repository ID", - SETTING_REMOTE_SEGMENT_STORE_REPOSITORY - ) - ) - ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testReplicationTypeDocumentByUser() throws Exception { @@ -174,19 +156,21 @@ public void testReplicationTypeDocumentByUser() throws Exception { .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) .put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT) .build(); - assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); - GetIndexResponse getIndexResponse = client().admin() - .indices() - .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) - .get(); - Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); - verifyRemoteStoreIndexSettings( - indexSettings, - null, - null, - null, - ReplicationType.DOCUMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() + ); + assertThat( + exc.getMessage(), + containsString( + String.format( + Locale.ROOT, + "To enable %s, %s should be set to %s", + SETTING_REMOTE_STORE_ENABLED, + SETTING_REPLICATION_TYPE, + ReplicationType.SEGMENT + ) + ) ); } @@ -222,20 +206,11 @@ public void testRemoteStoreEnabledByUserWithRemoteRepo() throws Exception { .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, "my-custom-repo") .build(); - assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); - GetIndexResponse getIndexResponse = client().admin() - .indices() - .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) - .get(); - Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); - verifyRemoteStoreIndexSettings( - indexSettings, - "true", - "my-custom-repo", - "my-translog-repo-1", - ReplicationType.SEGMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testRemoteStoreOverrideOnlyTranslogRepoIllegalArgumentException() throws Exception { @@ -270,20 +245,11 @@ public void testRemoteStoreOverrideTranslogRepoCorrectly() throws Exception { .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, "my-custom-repo") .put(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, "my-custom-repo") .build(); - assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); - GetIndexResponse getIndexResponse = client().admin() - .indices() - .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) - .get(); - Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); - verifyRemoteStoreIndexSettings( - indexSettings, - "true", - "my-custom-repo", - "my-custom-repo", - ReplicationType.SEGMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testRemoteStoreOverrideReplicationTypeIndexSettings() throws Exception { @@ -292,19 +258,21 @@ public void testRemoteStoreOverrideReplicationTypeIndexSettings() throws Excepti .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) .put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT) .build(); - assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); - GetIndexResponse getIndexResponse = client().admin() - .indices() - .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) - .get(); - Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); - verifyRemoteStoreIndexSettings( - indexSettings, - null, - null, - null, - ReplicationType.DOCUMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() + ); + assertThat( + exc.getMessage(), + containsString( + String.format( + Locale.ROOT, + "To enable %s, %s should be set to %s", + SETTING_REMOTE_STORE_ENABLED, + SETTING_REPLICATION_TYPE, + ReplicationType.SEGMENT + ) + ) ); } 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 728bac647d74a..76156e8234f3e 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -91,7 +91,6 @@ import org.opensearch.indices.InvalidIndexNameException; import org.opensearch.indices.ShardLimitValidator; import org.opensearch.indices.SystemIndices; -import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.threadpool.ThreadPool; import java.io.IOException; @@ -105,7 +104,6 @@ import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; @@ -954,44 +952,19 @@ private static void updateRemoteStoreSettings(Settings.Builder settingsBuilder, if (CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings)) { // Verify if we can create a remote store based index based on user provided settings if (canCreateRemoteStoreIndex(requestSettings) == false) { - return; + throw new IllegalArgumentException("Cannot override settings related to remote store."); } - // Verify index has replication type as SEGMENT - if (ReplicationType.DOCUMENT.equals(ReplicationType.parseString(settingsBuilder.get(SETTING_REPLICATION_TYPE)))) { - throw new IllegalArgumentException( - "Cannot enable [" - + SETTING_REMOTE_STORE_ENABLED - + "] when [" - + SETTING_REPLICATION_TYPE - + "] is " - + ReplicationType.DOCUMENT - ); - } - - settingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, true); - String remoteStoreRepo; - if (Objects.equals(requestSettings.get(INDEX_REMOTE_STORE_ENABLED_SETTING.getKey()), "true")) { - remoteStoreRepo = requestSettings.get(INDEX_REMOTE_STORE_REPOSITORY_SETTING.getKey()); - } else { - remoteStoreRepo = CLUSTER_REMOTE_STORE_REPOSITORY_SETTING.get(clusterSettings); - } - settingsBuilder.put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, remoteStoreRepo) - .put( - SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, - requestSettings.get( - INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.getKey(), - CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(clusterSettings) - ) - ); + settingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, true) + .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, CLUSTER_REMOTE_STORE_REPOSITORY_SETTING.get(clusterSettings)) + .put(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(clusterSettings)); } } private static boolean canCreateRemoteStoreIndex(Settings requestSettings) { - return (INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings) == false - || INDEX_REPLICATION_TYPE_SETTING.get(requestSettings).equals(ReplicationType.SEGMENT)) - && (INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == false - || INDEX_REMOTE_STORE_ENABLED_SETTING.get(requestSettings)); + return INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == false + && INDEX_REMOTE_STORE_REPOSITORY_SETTING.exists(requestSettings) == false + && INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.exists(requestSettings) == false; } public static void validateStoreTypeSettings(Settings settings) { diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java index e52237c8dba99..025e21ffb08cc 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -1296,24 +1296,20 @@ public void testRemoteStoreDisabledByUserIndexSettings() { final Settings.Builder requestSettings = Settings.builder(); requestSettings.put(SETTING_REMOTE_STORE_ENABLED, false); request.settings(requestSettings.build()); - Settings indexSettings = aggregateIndexSettings( - ClusterState.EMPTY_STATE, - request, - Settings.EMPTY, - null, - settings, - IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, - randomShardLimitService(), - Collections.emptySet() - ); - verifyRemoteStoreIndexSettings( - indexSettings, - "false", - null, - null, - ReplicationType.SEGMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> aggregateIndexSettings( + ClusterState.EMPTY_STATE, + request, + Settings.EMPTY, + null, + settings, + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, + randomShardLimitService(), + Collections.emptySet() + ) ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testRemoteStoreOverrideSegmentRepoIndexSettings() { @@ -1331,24 +1327,20 @@ public void testRemoteStoreOverrideSegmentRepoIndexSettings() { .put(SETTING_REMOTE_STORE_ENABLED, true) .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, "my-custom-repo"); request.settings(requestSettings.build()); - Settings indexSettings = aggregateIndexSettings( - ClusterState.EMPTY_STATE, - request, - Settings.EMPTY, - null, - settings, - IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, - randomShardLimitService(), - Collections.emptySet() - ); - verifyRemoteStoreIndexSettings( - indexSettings, - "true", - "my-custom-repo", - "my-translog-repo-1", - ReplicationType.SEGMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> aggregateIndexSettings( + ClusterState.EMPTY_STATE, + request, + Settings.EMPTY, + null, + settings, + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, + randomShardLimitService(), + Collections.emptySet() + ) ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testRemoteStoreOverrideTranslogRepoIndexSettings() { @@ -1364,24 +1356,20 @@ public void testRemoteStoreOverrideTranslogRepoIndexSettings() { final Settings.Builder requestSettings = Settings.builder(); requestSettings.put(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, "my-custom-repo"); request.settings(requestSettings.build()); - Settings indexSettings = aggregateIndexSettings( - ClusterState.EMPTY_STATE, - request, - Settings.EMPTY, - null, - settings, - IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, - randomShardLimitService(), - Collections.emptySet() - ); - verifyRemoteStoreIndexSettings( - indexSettings, - "true", - "my-segment-repo-1", - "my-custom-repo", - ReplicationType.SEGMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> aggregateIndexSettings( + ClusterState.EMPTY_STATE, + request, + Settings.EMPTY, + null, + settings, + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, + randomShardLimitService(), + Collections.emptySet() + ) ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testRemoteStoreOverrideReplicationTypeIndexSettings() { @@ -1397,24 +1385,20 @@ public void testRemoteStoreOverrideReplicationTypeIndexSettings() { final Settings.Builder requestSettings = Settings.builder(); requestSettings.put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT); request.settings(requestSettings.build()); - Settings indexSettings = aggregateIndexSettings( - ClusterState.EMPTY_STATE, - request, - Settings.EMPTY, - null, - settings, - IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, - randomShardLimitService(), - Collections.emptySet() - ); - verifyRemoteStoreIndexSettings( - indexSettings, - null, - null, - null, - ReplicationType.DOCUMENT.toString(), - IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL + IllegalArgumentException exc = expectThrows( + IllegalArgumentException.class, + () -> aggregateIndexSettings( + ClusterState.EMPTY_STATE, + request, + Settings.EMPTY, + null, + settings, + IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, + randomShardLimitService(), + Collections.emptySet() + ) ); + assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); } public void testBuildIndexMetadata() {