From e057f1a9dbca9f51397b830bf4625e2104dd6d2a Mon Sep 17 00:00:00 2001
From: Tanguy Leroux <tlrx.dev@gmail.com>
Date: Fri, 29 Jun 2018 14:15:34 +0200
Subject: [PATCH] Do not check for object existence when deleting repository
 index files (#31680)

Before deleting a repository index generation file, BlobStoreRepository
checks for the existence of the file and then deletes it. We can save
a request here by using BlobContainer.deleteBlobIgnoringIfNotExists()
 which ignores error when deleting a file that does not exist.

Since there is no way with S3 to know if a non versioned file existed
before being deleted, this pull request also changes S3BlobContainer so
that it now implements deleteBlobIgnoringIfNotExists(). It will now save
 one more request (blobExist?) when appropriate. The tests and fixture
 have been modified to conform the S3 API that always returns a 204/NO
 CONTENT HTTP response on deletions.
---
 .../repositories/s3/AmazonS3Fixture.java            |  6 ++----
 .../repositories/s3/S3BlobContainer.java            |  5 +++++
 .../elasticsearch/repositories/s3/MockAmazonS3.java | 10 ++--------
 .../blobstore/BlobStoreIndexShardSnapshot.java      | 13 -------------
 .../repositories/blobstore/BlobStoreRepository.java | 12 +++---------
 5 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java b/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java
index 20e21675acb79..d1034aff48248 100644
--- a/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java
+++ b/plugins/repository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java
@@ -174,10 +174,8 @@ private static PathTrie<RequestHandler> defaultHandlers(final Map<String, Bucket
                 }
 
                 final String objectName = objectName(request.getParameters());
-                if (bucket.objects.remove(objectName) != null) {
-                    return new Response(RestStatus.OK.getStatus(), TEXT_PLAIN_CONTENT_TYPE, EMPTY_BYTE);
-                }
-                return newObjectNotFoundError(request.getId(), objectName);
+                bucket.objects.remove(objectName);
+                return new Response(RestStatus.NO_CONTENT.getStatus(), TEXT_PLAIN_CONTENT_TYPE, EMPTY_BYTE);
             })
         );
 
diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java
index 102af6f9f5b4c..86b01a3e79cdd 100644
--- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java
+++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java
@@ -107,8 +107,13 @@ public void deleteBlob(String blobName) throws IOException {
         if (blobExists(blobName) == false) {
             throw new NoSuchFileException("Blob [" + blobName + "] does not exist");
         }
+        deleteBlobIgnoringIfNotExists(blobName);
+    }
 
+    @Override
+    public void deleteBlobIgnoringIfNotExists(String blobName) throws IOException {
         try (AmazonS3Reference clientReference = blobStore.clientReference()) {
+            // There is no way to know if an non-versioned object existed before the deletion
             SocketAccess.doPrivilegedVoid(() -> clientReference.client().deleteObject(blobStore.bucket(), buildKey(blobName)));
         } catch (final AmazonClientException e) {
             throw new IOException("Exception when deleting blob [" + blobName + "]", e);
diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java
index d610e6d74a06d..b5fb01869ae8c 100644
--- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java
+++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java
@@ -149,15 +149,9 @@ public ObjectListing listObjects(final ListObjectsRequest request) throws Amazon
     @Override
     public void deleteObject(final DeleteObjectRequest request) throws AmazonClientException {
         assertThat(request.getBucketName(), equalTo(bucket));
-
-        final String blobName = request.getKey();
-        if (blobs.remove(blobName) == null) {
-            AmazonS3Exception exception = new AmazonS3Exception("[" + blobName + "] does not exist.");
-            exception.setStatusCode(404);
-            throw exception;
-        }
+        blobs.remove(request.getKey());
     }
-    
+
     @Override
     public void shutdown() {
         // TODO check close
diff --git a/server/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java b/server/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java
index 275bc432942d3..297c12744cc26 100644
--- a/server/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java
+++ b/server/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java
@@ -389,19 +389,6 @@ public BlobStoreIndexShardSnapshot(String snapshot, long indexVersion, List<File
         this.incrementalSize = incrementalSize;
     }
 
-    /**
-     * Special constructor for the prototype
-     */
-    private BlobStoreIndexShardSnapshot() {
-        this.snapshot = "";
-        this.indexVersion = 0;
-        this.indexFiles = Collections.emptyList();
-        this.startTime = 0;
-        this.time = 0;
-        this.incrementalFileCount = 0;
-        this.incrementalSize = 0;
-    }
-
     /**
      * Returns index version
      *
diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
index 0326dccc14a8b..b8849bcd0b3f1 100644
--- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
+++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
@@ -668,9 +668,7 @@ protected void writeIndexGen(final RepositoryData repositoryData, final long rep
         // delete the N-2 index file if it exists, keep the previous one around as a backup
         if (isReadOnly() == false && newGen - 2 >= 0) {
             final String oldSnapshotIndexFile = INDEX_FILE_PREFIX + Long.toString(newGen - 2);
-            if (snapshotsBlobContainer.blobExists(oldSnapshotIndexFile)) {
-                snapshotsBlobContainer.deleteBlob(oldSnapshotIndexFile);
-            }
+            snapshotsBlobContainer.deleteBlobIgnoringIfNotExists(oldSnapshotIndexFile);
         }
 
         // write the current generation to the index-latest file
@@ -679,9 +677,7 @@ protected void writeIndexGen(final RepositoryData repositoryData, final long rep
             bStream.writeLong(newGen);
             genBytes = bStream.bytes();
         }
-        if (snapshotsBlobContainer.blobExists(INDEX_LATEST_BLOB)) {
-            snapshotsBlobContainer.deleteBlob(INDEX_LATEST_BLOB);
-        }
+        snapshotsBlobContainer.deleteBlobIgnoringIfNotExists(INDEX_LATEST_BLOB);
         logger.debug("Repository [{}] updating index.latest with generation [{}]", metadata.name(), newGen);
         writeAtomic(INDEX_LATEST_BLOB, genBytes);
     }
@@ -702,9 +698,7 @@ void writeIncompatibleSnapshots(RepositoryData repositoryData) throws IOExceptio
             }
             bytes = bStream.bytes();
         }
-        if (snapshotsBlobContainer.blobExists(INCOMPATIBLE_SNAPSHOTS_BLOB)) {
-            snapshotsBlobContainer.deleteBlob(INCOMPATIBLE_SNAPSHOTS_BLOB);
-        }
+        snapshotsBlobContainer.deleteBlobIgnoringIfNotExists(INCOMPATIBLE_SNAPSHOTS_BLOB);
         // write the incompatible snapshots blob
         writeAtomic(INCOMPATIBLE_SNAPSHOTS_BLOB, bytes);
     }