From 3da4a247219c444b69c501069aff2c10c81eda57 Mon Sep 17 00:00:00 2001
From: Armin Braun <me@obrown.io>
Date: Mon, 30 Mar 2020 09:47:24 +0200
Subject: [PATCH] Remove Redundant Cleanup Step in Snapshot Failure

Ever since we don't write any data to the repo during
snapshot initialization, the `snapshotCreated` flag has become obsolete.
If we run into any exception during snapshot init we should not redundantly
finalize an empty snapshot in the reopsitory.
---
 .../snapshots/SnapshotsService.java           | 49 +++----------------
 1 file changed, 6 insertions(+), 43 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
index 96cdf3a13b5e4..c4c16ac172352 100644
--- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
+++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
@@ -417,8 +417,6 @@ private void beginSnapshot(final ClusterState clusterState,
                                final ActionListener<Snapshot> userCreateSnapshotListener) {
         threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(new AbstractRunnable() {
 
-            boolean snapshotCreated;
-
             boolean hadAbortedInitializations;
 
             @Override
@@ -437,8 +435,6 @@ protected void doRun() {
                             repository.getMetadata().name(), snapshotName, "snapshot with the same name already exists");
                     }
 
-                    snapshotCreated = true;
-
                     logger.info("snapshot [{}] started", snapshot.snapshot());
                     final Version version =
                         minCompatibleVersion(clusterState.nodes().getMinNodeVersion(), snapshot.repository(), repositoryData, null);
@@ -508,7 +504,7 @@ public void onFailure(String source, Exception e) {
                             logger.warn(() -> new ParameterizedMessage("[{}] failed to create snapshot",
                                 snapshot.snapshot().getSnapshotId()), e);
                             removeSnapshotFromClusterState(snapshot.snapshot(), null, e,
-                                new CleanupAfterErrorListener(snapshot, true, userCreateSnapshotListener, e));
+                                new CleanupAfterErrorListener(userCreateSnapshotListener, e));
                         }
 
                         @Override
@@ -545,68 +541,35 @@ public void onFailure(Exception e) {
                 logger.warn(() -> new ParameterizedMessage("failed to create snapshot [{}]",
                     snapshot.snapshot().getSnapshotId()), e);
                 removeSnapshotFromClusterState(snapshot.snapshot(), null, e,
-                    new CleanupAfterErrorListener(snapshot, snapshotCreated, userCreateSnapshotListener, e));
+                    new CleanupAfterErrorListener(userCreateSnapshotListener, e));
             }
         });
     }
 
-    private class CleanupAfterErrorListener implements ActionListener<SnapshotInfo> {
+    private static class CleanupAfterErrorListener implements ActionListener<SnapshotInfo> {
 
-        private final SnapshotsInProgress.Entry snapshot;
-        private final boolean snapshotCreated;
         private final ActionListener<Snapshot> userCreateSnapshotListener;
         private final Exception e;
 
-        CleanupAfterErrorListener(SnapshotsInProgress.Entry snapshot, boolean snapshotCreated,
-                                  ActionListener<Snapshot> userCreateSnapshotListener, Exception e) {
-            this.snapshot = snapshot;
-            this.snapshotCreated = snapshotCreated;
+        CleanupAfterErrorListener(ActionListener<Snapshot> userCreateSnapshotListener, Exception e) {
             this.userCreateSnapshotListener = userCreateSnapshotListener;
             this.e = e;
         }
 
         @Override
         public void onResponse(SnapshotInfo snapshotInfo) {
-            cleanupAfterError(this.e);
+            userCreateSnapshotListener.onFailure(e);
         }
 
         @Override
         public void onFailure(Exception e) {
             e.addSuppressed(this.e);
-            cleanupAfterError(e);
+            userCreateSnapshotListener.onFailure(e);
         }
 
         public void onNoLongerMaster() {
             userCreateSnapshotListener.onFailure(e);
         }
-
-        private void cleanupAfterError(Exception exception) {
-            threadPool.generic().execute(() -> {
-                if (snapshotCreated) {
-                    final MetaData metaData = clusterService.state().metaData();
-                    repositoriesService.repository(snapshot.snapshot().getRepository())
-                        .finalizeSnapshot(snapshot.snapshot().getSnapshotId(),
-                            buildGenerations(snapshot, metaData),
-                            snapshot.startTime(),
-                            ExceptionsHelper.stackTrace(exception),
-                            0,
-                            Collections.emptyList(),
-                            snapshot.repositoryStateId(),
-                            snapshot.includeGlobalState(),
-                            metaDataForSnapshot(snapshot, metaData),
-                            snapshot.userMetadata(),
-                            snapshot.version(),
-                            ActionListener.runAfter(ActionListener.wrap(ignored -> {
-                            }, inner -> {
-                                inner.addSuppressed(exception);
-                                logger.warn(() -> new ParameterizedMessage("[{}] failed to finalize snapshot in repository",
-                                    snapshot.snapshot()), inner);
-                            }), () -> userCreateSnapshotListener.onFailure(e)));
-                } else {
-                    userCreateSnapshotListener.onFailure(e);
-                }
-            });
-        }
     }
 
     private static ShardGenerations buildGenerations(SnapshotsInProgress.Entry snapshot, MetaData metaData) {