From 0e78d5fe61d80811e97f5132204c9d82f260aa89 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sun, 1 Aug 2021 20:58:03 +0200 Subject: [PATCH 1/2] Fix Needless Warnings when Restoring over Closed Index 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. --- .../snapshots/RestoreSnapshotIT.java | 36 +++++++++++++++++++ .../blobstore/FileRestoreContext.java | 2 +- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java index 99a2290784ad0..906be39192d75 100644 --- 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/repositories/blobstore/FileRestoreContext.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java index b4d3ab65bed4f..f5c817a0adefe 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java @@ -188,12 +188,12 @@ private void afterRestore(SnapshotFiles snapshotFiles, Store store, StoreFileMet /// now, go over and clean files that are in the store, but were not in the snapshot try { + store.deleteQuiet("restore"); 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 } 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 From 9de37aa39d2f40017f2d2263ecff3df6743b7c4c Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 2 Aug 2021 10:52:47 +0200 Subject: [PATCH 2/2] CR comments --- server/src/main/java/org/elasticsearch/index/store/Store.java | 2 +- .../repositories/blobstore/FileRestoreContext.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/store/Store.java b/server/src/main/java/org/elasticsearch/index/store/Store.java index 45d0850171b9a..3bae95ebbeff5 100644 --- 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 index f5c817a0adefe..dad6f296512f1 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java @@ -188,10 +188,9 @@ private void afterRestore(SnapshotFiles snapshotFiles, Store store, StoreFileMet /// now, go over and clean files that are in the store, but were not in the snapshot try { - store.deleteQuiet("restore"); 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.directory().deleteFile(storeFile);