address PR comments

Signed-off-by: bansvaru <[email protected]>
opensearch-project · Oct 20, 2023 · 602a79d · 602a79d
1 parent 98b15da
commit 602a79d
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 36 deletions.
diff --git a/server/src/main/java/org/opensearch/gateway/GatewayMetaState.java b/server/src/main/java/org/opensearch/gateway/GatewayMetaState.java
@@ -175,7 +175,6 @@ public void start(
                             if (ClusterState.UNKNOWN_UUID.equals(lastKnownClusterUUID) == false) {
                                 // Load state from remote
                                 final RemoteRestoreResult remoteRestoreResult = remoteStoreRestoreService.restore(
-                                    // Override Metadata restored from local disk during remote state restore.
                                     // Remote Metadata should always override local disk Metadata
                                     // if local disk Metadata's cluster uuid is UNKNOWN_UUID
                                     ClusterState.builder(clusterState).metadata(Metadata.EMPTY_METADATA).build(),
@@ -553,7 +552,9 @@ static class LucenePersistedState implements PersistedState {
             // out by this version of OpenSearch. TODO TBD should we avoid indexing when possible?
             final PersistedClusterStateService.Writer writer = persistedClusterStateService.createWriter();
             try {
-                // With Remote State we can write cluster state with non empty Metadata but UNKNOWN_UUID cluster uuid
+                // During remote state restore, there will be non empty metadata getting persisted with cluster UUID as
+                // ClusterState.UNKOWN_UUID . The valid UUID will be generated and persisted along with the first cluster state getting
+                // published.
                 writer.writeFullStateAndCommit(currentTerm, lastAcceptedState);
             } catch (Exception e) {
                 try {

diff --git a/server/src/main/java/org/opensearch/index/recovery/RemoteStoreRestoreService.java b/server/src/main/java/org/opensearch/index/recovery/RemoteStoreRestoreService.java
@@ -144,6 +144,11 @@ public RemoteRestoreResult restore(
             || restoreClusterUUID.isBlank()) == false;
         if (metadataFromRemoteStore) {
             try {
+                // Restore with current cluster UUID will fail as same indices would be present in the cluster which we are trying to
+                // restore
+                if (currentState.metadata().clusterUUID().equals(restoreClusterUUID)) {
+                    throw new IllegalArgumentException("clusterUUID to restore from should be different from current cluster UUID");
+                }
                 remoteMetadata = remoteClusterStateService.getLatestMetadata(currentState.getClusterName().value(), restoreClusterUUID);
                 remoteMetadata.getIndices().values().forEach(indexMetadata -> {
                     indexMetadataMap.put(indexMetadata.getIndex().getName(), new Tuple<>(true, indexMetadata));
@@ -156,12 +161,21 @@ public RemoteRestoreResult restore(
                 IndexMetadata indexMetadata = currentState.metadata().index(indexName);
                 if (indexMetadata == null) {
                     logger.warn("Index restore is not supported for non-existent index. Skipping: {}", indexName);
+                } else if (indexMetadata.getSettings().getAsBoolean(SETTING_REMOTE_STORE_ENABLED, false)) {
+                    logger.warn("Remote store is not enabled for index: {}", indexName);
+                } else if (restoreAllShards && IndexMetadata.State.CLOSE.equals(indexMetadata.getState()) == false) {
+                    throw new IllegalStateException(
+                        String.format(
+                            Locale.ROOT,
+                            "cannot restore index [%s] because an open index with same name/uuid already exists in the cluster.",
+                            indexName
+                        ) + " Close the existing index."
+                    );
                 } else {
                     indexMetadataMap.put(indexName, new Tuple<>(false, indexMetadata));
                 }
             }
         }
-        validate(currentState, indexMetadataMap, restoreClusterUUID, restoreAllShards);
         return executeRestore(currentState, indexMetadataMap, restoreAllShards, remoteMetadata);
     }
 
@@ -270,39 +284,6 @@ private void restoreGlobalMetadata(Metadata.Builder mdBuilder, Metadata remoteMe
         repositoriesMetadata.ifPresent(metadata -> mdBuilder.putCustom(RepositoriesMetadata.TYPE, metadata));
     }
 
-    /**
-     * Performs various validations needed before executing restore
-     * @param currentState current cluster state
-     * @param indexMetadataMap map of index metadata to restore
-     * @param restoreClusterUUID cluster UUID used to restore IndexMetadata
-     * @param restoreAllShards indicates if all shards of the index needs to be restored. This flat is ignored if remoteClusterUUID is provided
-     */
-    private void validate(
-        ClusterState currentState,
-        Map<String, Tuple<Boolean, IndexMetadata>> indexMetadataMap,
-        @Nullable String restoreClusterUUID,
-        boolean restoreAllShards
-    ) throws IllegalStateException, IllegalArgumentException {
-        String errorMsg = "cannot restore index [%s] because an open index with same name/uuid already exists in the cluster.";
-
-        // Restore with current cluster UUID will fail as same indices would be present in the cluster which we are trying to
-        // restore
-        if (currentState.metadata().clusterUUID().equals(restoreClusterUUID)) {
-            throw new IllegalArgumentException("clusterUUID to restore from should be different from current cluster UUID");
-        }
-        for (Map.Entry<String, Tuple<Boolean, IndexMetadata>> indexMetadataEntry : indexMetadataMap.entrySet()) {
-            String indexName = indexMetadataEntry.getKey();
-            IndexMetadata indexMetadata = indexMetadataEntry.getValue().v2();
-            if (indexMetadata.getSettings().getAsBoolean(SETTING_REMOTE_STORE_ENABLED, false)) {
-                if (restoreAllShards && IndexMetadata.State.CLOSE.equals(indexMetadata.getState()) == false) {
-                    throw new IllegalStateException(String.format(Locale.ROOT, errorMsg, indexName) + " Close the existing index.");
-                }
-            } else {
-                logger.warn("Remote store is not enabled for index: {}", indexName);
-            }
-        }
-    }
-
     /**
      * Result of a remote restore operation.
      */