Fix Needless Warnings when Restoring over Closed Index (#75912)

We're trying to delete each file twice and will always warn on the second non-suppressing delete method when there's files to delete.
elastic · Aug 2, 2021 · 6d99735 · 6d99735
1 parent 09a5db3
commit 6d99735
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 3 deletions.
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java
@@ -8,6 +8,9 @@
 
 package org.elasticsearch.snapshots;
 
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionFuture;
 import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
@@ -18,14 +21,17 @@
 import org.elasticsearch.cluster.block.ClusterBlocks;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.metadata.MappingMetadata;
+import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.indices.InvalidIndexNameException;
 import org.elasticsearch.repositories.RepositoriesService;
+import org.elasticsearch.repositories.blobstore.FileRestoreContext;
 import org.elasticsearch.repositories.fs.FsRepository;
 import org.elasticsearch.rest.RestStatus;
+import org.elasticsearch.test.MockLogAppender;
 
 import java.nio.file.Path;
 import java.util.Arrays;
@@ -876,4 +882,34 @@ public void testFailOnAncientVersion() throws Exception {
             )
         );
     }
+
+    public void testNoWarningsOnRestoreOverClosedIndex() throws IllegalAccessException {
+        final String repoName = "test-repo";
+        createRepository(repoName, FsRepository.TYPE);
+        final String indexName = "test-idx";
+        createIndexWithContent(indexName);
+        final String snapshotName = "test-snapshot";
+        createSnapshot(repoName, snapshotName, List.of(indexName));
+        index(indexName, "some_id", Map.of("foo", "bar"));
+        assertAcked(admin().indices().prepareClose(indexName).get());
+        final MockLogAppender mockAppender = new MockLogAppender();
+        mockAppender.addExpectation(
+            new MockLogAppender.UnseenEventExpectation("no warnings", FileRestoreContext.class.getCanonicalName(), Level.WARN, "*")
+        );
+        mockAppender.start();
+        final Logger logger = LogManager.getLogger(FileRestoreContext.class);
+        Loggers.addAppender(logger, mockAppender);
+        try {
+            final RestoreSnapshotResponse restoreSnapshotResponse = clusterAdmin().prepareRestoreSnapshot(repoName, snapshotName)
+                .setIndices(indexName)
+                .setRestoreGlobalState(false)
+                .setWaitForCompletion(true)
+                .get();
+            assertEquals(0, restoreSnapshotResponse.getRestoreInfo().failedShards());
+            mockAppender.assertAllExpectationsMatched();
+        } finally {
+            Loggers.removeAppender(logger, mockAppender);
+            mockAppender.stop();
+        }
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/store/Store.java b/server/src/main/java/org/elasticsearch/index/store/Store.java
@@ -1116,7 +1116,7 @@ public String toString() {
 
     /**
      * Returns true if the file is auto-generated by the store and shouldn't be deleted during cleanup.
-     * This includes write lock and checksum files
+     * This includes write lock files
      */
     public static boolean isAutogenerated(String name) {
         return IndexWriter.WRITE_LOCK_NAME.equals(name);

diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java
@@ -190,10 +190,9 @@ private void afterRestore(SnapshotFiles snapshotFiles, Store store, StoreFileMet
         try {
             for (String storeFile : store.directory().listAll()) {
                 if (Store.isAutogenerated(storeFile) || snapshotFiles.containPhysicalIndexFile(storeFile)) {
-                    continue; // skip write.lock, checksum files and files that exist in the snapshot
+                    continue; // skip write.lock and files that exist in the snapshot
                 }
                 try {
-                    store.deleteQuiet("restore", storeFile);
                     store.directory().deleteFile(storeFile);
                 } catch (ImmutableDirectoryException e) {
                     // snapshots of immutable directories only contain an empty `segments_N` file since the data lives elsewhere, and if we