Addressing comments

Signed-off-by: Shourya Dutta Biswas <114977491+shourya035@users.noreply.github.com>
opensearch-project · Apr 25, 2024 · 98004f8 · 98004f8
1 parent 5c8df2c
commit 98004f8
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 21 deletions.
diff --git a/server/src/main/java/org/opensearch/cluster/routing/allocation/IndexMetadataUpdater.java b/server/src/main/java/org/opensearch/cluster/routing/allocation/IndexMetadataUpdater.java
@@ -60,8 +60,6 @@
 import java.util.stream.Collectors;
 
 import static org.opensearch.index.remote.RemoteStoreUtils.getRemoteStoreRepoName;
-import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY;
-import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY;
 
 /**
  * Observer that tracks changes made to RoutingNodes in order to update the primary terms and in-sync allocation ids in
@@ -185,12 +183,7 @@ public Metadata applyChanges(Metadata oldMetadata, RoutingTable newRoutingTable,
                         logger
                     );
                     migrationImdUpdater.maybeUpdateRemoteStorePathStrategy(indexMetadataBuilder, index.getName());
-                    migrationImdUpdater.maybeAddRemoteIndexSettings(
-                        indexMetadataBuilder,
-                        index.getName(),
-                        remoteRepoNames.get(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY),
-                        remoteRepoNames.get(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY)
-                    );
+                    migrationImdUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, index.getName());
                 }
             }
 

diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteMigrationIndexMetadataUpdater.java b/server/src/main/java/org/opensearch/index/remote/RemoteMigrationIndexMetadataUpdater.java
@@ -32,8 +32,11 @@
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED;
 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.remote.RemoteStoreUtils.getRemoteStoreRepoName;
 import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_STORE_PATH_HASH_ALGORITHM_SETTING;
 import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_STORE_PATH_TYPE_SETTING;
+import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY;
+import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY;
 
 /**
  * Utils for checking and mutating cluster state during remote migration
@@ -68,18 +71,16 @@ public RemoteMigrationIndexMetadataUpdater(
      * <br>
      * Also appends the requisite Remote Store Path based custom metadata to the existing index metadata
      */
-    public void maybeAddRemoteIndexSettings(
-        IndexMetadata.Builder indexMetadataBuilder,
-        String index,
-        String segmentRepoName,
-        String tlogRepoName
-    ) {
+    public void maybeAddRemoteIndexSettings(IndexMetadata.Builder indexMetadataBuilder, String index) {
         Settings currentIndexSettings = indexMetadata.getSettings();
         if (needsRemoteIndexSettingsUpdate(routingTable.indicesRouting().get(index), discoveryNodes, currentIndexSettings)) {
             logger.info(
                 "Index {} does not have remote store based index settings but all primary shards and STARTED replica shards have moved to remote enabled nodes. Applying remote store settings to the index",
                 index
             );
+            Map<String, String> remoteRepoNames = getRemoteStoreRepoName(discoveryNodes);
+            String segmentRepoName = remoteRepoNames.get(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY);
+            String tlogRepoName = remoteRepoNames.get(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY);
             assert Objects.nonNull(segmentRepoName) && Objects.nonNull(tlogRepoName) : "Remote repo names cannot be null";
             Settings.Builder indexSettingsBuilder = Settings.builder().put(currentIndexSettings);
             updateRemoteStoreSettings(indexSettingsBuilder, segmentRepoName, tlogRepoName);

diff --git a/...r/src/test/java/org/opensearch/index/remote/RemoteMigrationIndexMetadataUpdaterTests.java b/...r/src/test/java/org/opensearch/index/remote/RemoteMigrationIndexMetadataUpdaterTests.java
@@ -38,8 +38,6 @@
 import static org.mockito.Mockito.mock;
 
 public class RemoteMigrationIndexMetadataUpdaterTests extends OpenSearchTestCase {
-    private final String tlogRepoName = "test-tlog-repo";
-    private final String segmentRepoName = "test-segment-repo";
     private final String indexName = "test-index";
 
     public void testMaybeAddRemoteIndexSettingsAllPrimariesAndReplicasOnRemote() throws IOException {
@@ -58,7 +56,7 @@ public void testMaybeAddRemoteIndexSettingsAllPrimariesAndReplicasOnRemote() thr
             metadata.settings(),
             logger
         );
-        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
+        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName);
         assertTrue(currentSettingsVersion < indexMetadataBuilder.settingsVersion());
         assertRemoteSettingsApplied(indexMetadataBuilder.build());
     }
@@ -79,7 +77,7 @@ public void testMaybeAddRemoteIndexSettingsDoesNotRunWhenSettingsAlreadyPresent(
             metadata.settings(),
             logger
         );
-        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
+        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName);
         assertEquals(currentSettingsVersion, indexMetadataBuilder.settingsVersion());
     }
 
@@ -99,7 +97,7 @@ public void testMaybeAddRemoteIndexSettingsDoesNotUpdateSettingsWhenAllShardsInD
             metadata.settings(),
             logger
         );
-        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
+        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName);
         assertEquals(currentSettingsVersion, indexMetadataBuilder.settingsVersion());
         assertDocrepSettingsApplied(indexMetadataBuilder.build());
     }
@@ -120,7 +118,7 @@ public void testMaybeAddRemoteIndexSettingsUpdatesIndexSettingsWithUnassignedRep
             metadata.settings(),
             logger
         );
-        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
+        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName);
         assertTrue(currentSettingsVersion < indexMetadataBuilder.settingsVersion());
         assertRemoteSettingsApplied(indexMetadataBuilder.build());
     }
@@ -142,7 +140,7 @@ public void testMaybeAddRemoteIndexSettingsDoesNotUpdateIndexSettingsWithRelocat
             metadata.settings(),
             logger
         );
-        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName, segmentRepoName, tlogRepoName);
+        migrationIndexMetadataUpdater.maybeAddRemoteIndexSettings(indexMetadataBuilder, indexName);
         assertEquals(currentSettingsVersion, indexMetadataBuilder.settingsVersion());
         assertDocrepSettingsApplied(indexMetadataBuilder.build());
     }