[Segment Replication + Remote Store] Removing exception throw when re…

…mote store is used with system indices (opensearch-project#8235) * Removing throwing of exception when remote store is used with system indices. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Add check of feature flag enabled. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Add unit test to improve code coverage. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Add warning log. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> --------- Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
mch2 · Jun 23, 2023 · 3b04ef4 · 3b04ef4
1 parent f21d73e
commit 3b04ef4
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 27 deletions.
diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java
@@ -12,6 +12,8 @@
 import org.junit.Before;
 import org.opensearch.action.admin.indices.get.GetIndexRequest;
 import org.opensearch.action.admin.indices.get.GetIndexResponse;
+import org.opensearch.action.admin.indices.settings.get.GetSettingsRequest;
+import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
@@ -131,21 +133,30 @@ public void testDefaultRemoteStoreNoUserOverride() throws Exception {
     private static final String SYSTEM_INDEX_NAME = ".test-system-index";
 
     public void testSystemIndexWithRemoteStoreClusterSetting() throws Exception {
-        IllegalArgumentException illegalArgumentException = expectThrows(
-            IllegalArgumentException.class,
-            () -> createIndex(SYSTEM_INDEX_NAME)
-        );
-        assertThat(
-            illegalArgumentException.getMessage(),
-            containsString(
-                "Cannot enable ["
-                    + SETTING_REMOTE_STORE_ENABLED
-                    + "] when ["
-                    + SETTING_REPLICATION_TYPE
-                    + "] is "
-                    + ReplicationType.DOCUMENT
-            )
-        );
+        createIndex(SYSTEM_INDEX_NAME);
+        ensureGreen(SYSTEM_INDEX_NAME);
+        final GetSettingsResponse response = client().admin()
+            .indices()
+            .getSettings(new GetSettingsRequest().indices(SYSTEM_INDEX_NAME).includeDefaults(true))
+            .actionGet();
+        // Verify that Document replication strategy is used
+        assertEquals(response.getSetting(SYSTEM_INDEX_NAME, SETTING_REPLICATION_TYPE), ReplicationType.DOCUMENT.toString());
+        assertEquals(response.getSetting(SYSTEM_INDEX_NAME, SETTING_REMOTE_STORE_ENABLED), "false");
+    }
+
+    public void testSystemIndexWithRemoteStoreIndexSettings() throws Exception {
+        prepareCreate(
+            SYSTEM_INDEX_NAME,
+            Settings.builder().put(SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT).put(SETTING_REMOTE_STORE_ENABLED, true)
+        ).get();
+        ensureGreen(SYSTEM_INDEX_NAME);
+        final GetSettingsResponse response = client().admin()
+            .indices()
+            .getSettings(new GetSettingsRequest().indices(SYSTEM_INDEX_NAME).includeDefaults(true))
+            .actionGet();
+        // Verify that Document replication strategy is used
+        assertEquals(response.getSetting(SYSTEM_INDEX_NAME, SETTING_REPLICATION_TYPE), ReplicationType.DOCUMENT.toString());
+        assertEquals(response.getSetting(SYSTEM_INDEX_NAME, SETTING_REMOTE_STORE_ENABLED), "false");
     }
 
     public void testRemoteStoreDisabledByUser() throws Exception {

diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
@@ -72,6 +72,7 @@
 import org.opensearch.common.settings.IndexScopedSettings;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.common.xcontent.XContentHelper;
 import org.opensearch.core.common.Strings;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
@@ -136,6 +137,7 @@
 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.cluster.metadata.Metadata.DEFAULT_REPLICA_COUNT_SETTING;
+import static org.opensearch.common.util.FeatureFlags.REMOTE_STORE;
 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;
@@ -898,8 +900,18 @@ static Settings aggregateIndexSettings(
         indexSettingsBuilder.put(IndexMetadata.SETTING_INDEX_PROVIDED_NAME, request.getProvidedName());
         indexSettingsBuilder.put(SETTING_INDEX_UUID, UUIDs.randomBase64UUID());
 
-        updateReplicationStrategy(indexSettingsBuilder, request.settings(), settings, isSystemIndex);
-        updateRemoteStoreSettings(indexSettingsBuilder, request.settings(), settings);
+        if (isSystemIndex || IndexMetadata.INDEX_HIDDEN_SETTING.get(request.settings())) {
+            logger.warn(
+                "Setting replication.type: DOCUMENT will be used for Index until Segment Replication supports System and Hidden indices"
+            );
+            indexSettingsBuilder.put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT);
+            if (FeatureFlags.isEnabled(REMOTE_STORE)) {
+                indexSettingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, false);
+            }
+        } else {
+            updateReplicationStrategy(indexSettingsBuilder, request.settings(), settings);
+            updateRemoteStoreSettings(indexSettingsBuilder, request.settings(), settings);
+        }
 
         if (sourceMetadata != null) {
             assert request.resizeType() != null;
@@ -939,16 +951,7 @@ static Settings aggregateIndexSettings(
      * @param requestSettings settings passed in during index create request
      * @param clusterSettings cluster level settings
      */
-    private static void updateReplicationStrategy(
-        Settings.Builder settingsBuilder,
-        Settings requestSettings,
-        Settings clusterSettings,
-        boolean isSystemIndex
-    ) {
-        if (isSystemIndex || IndexMetadata.INDEX_HIDDEN_SETTING.get(requestSettings)) {
-            settingsBuilder.put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT);
-            return;
-        }
+    private static void updateReplicationStrategy(Settings.Builder settingsBuilder, Settings requestSettings, Settings clusterSettings) {
         if (CLUSTER_REPLICATION_TYPE_SETTING.exists(clusterSettings) && INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings) == false) {
             settingsBuilder.put(SETTING_REPLICATION_TYPE, CLUSTER_REPLICATION_TYPE_SETTING.get(clusterSettings));
             return;

diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java
@@ -1715,6 +1715,36 @@ public void testSystemIndexUsesDocumentReplication() {
         assertEquals(ReplicationType.DOCUMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE));
     }
 
+    public void testRemoteStoreDisabledForSystemIndices() {
+        Settings settings = Settings.builder()
+            .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT)
+            .put(CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey(), true)
+            .put(CLUSTER_REMOTE_STORE_REPOSITORY_SETTING.getKey(), "my-segment-repo-1")
+            .put(CLUSTER_REMOTE_TRANSLOG_STORE_ENABLED_SETTING.getKey(), true)
+            .put(CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING.getKey(), "my-translog-repo-1")
+            .build();
+        FeatureFlagSetter.set(FeatureFlags.REMOTE_STORE);
+
+        request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test");
+        final Settings.Builder requestSettings = Settings.builder();
+        request.settings(requestSettings.build());
+        // set isSystemIndex parameter as true
+        Settings indexSettings = aggregateIndexSettings(
+            ClusterState.EMPTY_STATE,
+            request,
+            Settings.EMPTY,
+            null,
+            settings,
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
+            randomShardLimitService(),
+            Collections.emptySet(),
+            true
+        );
+        // Verify that remote store is disabled.
+        assertEquals(indexSettings.get(SETTING_REMOTE_STORE_ENABLED), "false");
+        assertEquals(ReplicationType.DOCUMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE));
+    }
+
     private IndexTemplateMetadata addMatchingTemplate(Consumer<IndexTemplateMetadata.Builder> configurator) {
         IndexTemplateMetadata.Builder builder = templateMetadataBuilder("template1", "te*");
         configurator.accept(builder);