Incorporate PR review comments

Signed-off-by: Ashish Singh <[email protected]>
opensearch-project · Sep 26, 2023 · 8327634 · 8327634
1 parent 9dbe0a4
commit 8327634
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 16 deletions.
diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java
@@ -24,7 +24,7 @@
 import org.opensearch.index.IndexService;
 import org.opensearch.index.IndexSettings;
 import org.opensearch.index.shard.IndexShard;
-import org.opensearch.index.translog.Translog;
+import org.opensearch.index.translog.Translog.Durability;
 import org.opensearch.indices.IndicesService;
 import org.opensearch.indices.recovery.RecoveryState;
 import org.opensearch.plugins.Plugin;
@@ -324,17 +324,36 @@ public void testClusterBufferIntervalValidation() {
         );
     }
 
-    public void testAnyTranslogDurabilityWhenRestrictSettingFalse() throws ExecutionException, InterruptedException {
+    public void testRequestDurabilityWhenRestrictSettingExplicitFalse() throws ExecutionException, InterruptedException {
+        // Explicit node settings and request durability
+        testRestrictSettingFalse(true, Durability.REQUEST);
+    }
+
+    public void testAsyncDurabilityWhenRestrictSettingExplicitFalse() throws ExecutionException, InterruptedException {
+        // Explicit node settings and async durability
+        testRestrictSettingFalse(true, Durability.ASYNC);
+    }
+
+    public void testRequestDurabilityWhenRestrictSettingImplicitFalse() throws ExecutionException, InterruptedException {
+        // No node settings and request durability
+        testRestrictSettingFalse(false, Durability.REQUEST);
+    }
+
+    public void testAsyncDurabilityWhenRestrictSettingImplicitFalse() throws ExecutionException, InterruptedException {
+        // No node settings and async durability
+        testRestrictSettingFalse(false, Durability.ASYNC);
+    }
+
+    private void testRestrictSettingFalse(boolean setRestrictFalse, Durability durability) throws ExecutionException, InterruptedException {
         String clusterManagerName;
-        if (randomBoolean()) {
+        if (setRestrictFalse) {
             clusterManagerName = internalCluster().startClusterManagerOnlyNode(
                 Settings.builder().put(IndicesService.CLUSTER_REMOTE_INDEX_RESTRICT_ASYNC_DURABILITY_SETTING.getKey(), false).build()
             );
         } else {
             clusterManagerName = internalCluster().startClusterManagerOnlyNode();
         }
         String dataNode = internalCluster().startDataOnlyNodes(1).get(0);
-        Translog.Durability durability = randomFrom(Translog.Durability.values());
         Settings indexSettings = Settings.builder()
             .put(indexSettings())
             .put(IndexSettings.INDEX_TRANSLOG_DURABILITY_SETTING.getKey(), durability)
@@ -343,7 +362,7 @@ public void testAnyTranslogDurabilityWhenRestrictSettingFalse() throws Execution
         IndexShard indexShard = getIndexShard(dataNode);
         assertEquals(durability, indexShard.indexSettings().getTranslogDurability());
 
-        durability = randomFrom(Translog.Durability.values());
+        durability = randomFrom(Durability.values());
         client(clusterManagerName).admin()
             .indices()
             .updateSettings(
@@ -366,15 +385,15 @@ public void testAsyncDurabilityThrowsExceptionWhenRestrictSettingTrue() throws E
         // Case 1 - Test create index fails
         Settings indexSettings = Settings.builder()
             .put(indexSettings())
-            .put(IndexSettings.INDEX_TRANSLOG_DURABILITY_SETTING.getKey(), Translog.Durability.ASYNC)
+            .put(IndexSettings.INDEX_TRANSLOG_DURABILITY_SETTING.getKey(), Durability.ASYNC)
             .build();
         IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> createIndex(INDEX_NAME, indexSettings));
         assertEquals(expectedExceptionMsg, exception.getMessage());
 
         // Case 2 - Test update index fails
         createIndex(INDEX_NAME);
         IndexShard indexShard = getIndexShard(dataNode);
-        assertEquals(Translog.Durability.REQUEST, indexShard.indexSettings().getTranslogDurability());
+        assertEquals(Durability.REQUEST, indexShard.indexSettings().getTranslogDurability());
         exception = assertThrows(
             IllegalArgumentException.class,
             () -> client(clusterManagerName).admin()

diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
@@ -923,9 +923,7 @@ static Settings aggregateIndexSettings(
         validateTranslogRetentionSettings(indexSettings);
         validateStoreTypeSettings(indexSettings);
         validateRefreshIntervalSettings(request.settings(), clusterSettings);
-        if (isRemoteStoreAttributePresent(settings)) {
-            validateTranslogDurabilitySettings(request.settings(), clusterSettings);
-        }
+        validateTranslogDurabilitySettings(request.settings(), clusterSettings, settings);
 
         return indexSettings;
     }
@@ -1521,8 +1519,9 @@ static void validateRefreshIntervalSettings(Settings requestSettings, ClusterSet
      * @param requestSettings settings passed in during index create/update request
      * @param clusterSettings cluster setting
      */
-    static void validateTranslogDurabilitySettings(Settings requestSettings, ClusterSettings clusterSettings) {
-        if (IndexSettings.INDEX_TRANSLOG_DURABILITY_SETTING.exists(requestSettings) == false
+    static void validateTranslogDurabilitySettings(Settings requestSettings, ClusterSettings clusterSettings, Settings settings) {
+        if (isRemoteStoreAttributePresent(settings) == false
+            || IndexSettings.INDEX_TRANSLOG_DURABILITY_SETTING.exists(requestSettings) == false
             || clusterSettings.get(IndicesService.CLUSTER_REMOTE_INDEX_RESTRICT_ASYNC_DURABILITY_SETTING) == false) {
             return;
         }

diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
@@ -77,7 +77,6 @@
 import static org.opensearch.cluster.metadata.MetadataCreateIndexService.validateTranslogDurabilitySettings;
 import static org.opensearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX;
 import static org.opensearch.index.IndexSettings.same;
-import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.isRemoteStoreAttributePresent;
 
 /**
  * Service responsible for submitting update index settings requests
@@ -131,9 +130,7 @@ public void updateSettings(
             .build();
 
         validateRefreshIntervalSettings(normalizedSettings, clusterService.getClusterSettings());
-        if (isRemoteStoreAttributePresent(clusterService.getSettings())) {
-            validateTranslogDurabilitySettings(normalizedSettings, clusterService.getClusterSettings());
-        }
+        validateTranslogDurabilitySettings(normalizedSettings, clusterService.getClusterSettings(), clusterService.getSettings());
 
         Settings.Builder settingsForClosedIndices = Settings.builder();
         Settings.Builder settingsForOpenIndices = Settings.builder();