Remove Unused Single Delete in BlobStoreRepository (#50024) (#50123)

* Remove Unused Single Delete in BlobStoreRepository

There are no more production uses of the non-bulk delete or the delete that throws
on missing so this commit removes both these methods.
Only the bulk delete logic remains. Where the bulk delete was derived from single deletes,
the single delete code was inlined into the bulk delete method.
Where single delete was used in tests it was replaced by bulk deleting.

modules/repository-url/src/main/java/org/elasticsearch/common/blobstore/url/URLBlobContainer.java

@@ -35,6 +35,7 @@ import java.nio.file.NoSuchFileException;
 import java.security.AccessController;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -93,7 +94,7 @@ public class URLBlobContainer extends AbstractBlobContainer {
      * This operation is not supported by URLBlobContainer
      */
     @Override
-    public void deleteBlob(String blobName) throws IOException {
+    public void deleteBlobsIgnoringIfNotExists(List<String> blobNames) {
         throw new UnsupportedOperationException("URL repository is read only");
     }
 

plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobContainer.java

@@ -109,22 +109,6 @@ public class AzureBlobContainer extends AbstractBlobContainer {
         writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists);
     }
 
-    @Override
-    public void deleteBlob(String blobName) throws IOException {
-        logger.trace("deleteBlob({})", blobName);
-
-        try {
-            blobStore.deleteBlob(buildKey(blobName));
-        } catch (StorageException e) {
-            if (e.getHttpStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
-                throw new NoSuchFileException(e.getMessage());
-            }
-            throw new IOException(e);
-        } catch (URISyntaxException e) {
-            throw new IOException(e);
-        }
-    }
-
     @Override
     public DeleteResult delete() throws IOException {
         try {
@@ -146,7 +130,18 @@ public class AzureBlobContainer extends AbstractBlobContainer {
             // Executing deletes in parallel since Azure SDK 8 is using blocking IO while Azure does not provide a bulk delete API endpoint
             // TODO: Upgrade to newer non-blocking Azure SDK 11 and execute delete requests in parallel that way.
             for (String blobName : blobNames) {
-                executor.execute(ActionRunnable.run(listener, () -> deleteBlobIgnoringIfNotExists(blobName)));
+                executor.execute(ActionRunnable.run(listener, () -> {
+                    logger.trace("deleteBlob({})", blobName);
+                    try {
+                        blobStore.deleteBlob(buildKey(blobName));
+                    } catch (StorageException e) {
+                        if (e.getHttpStatusCode() != HttpURLConnection.HTTP_NOT_FOUND) {
+                            throw new IOException(e);
+                        }
+                    } catch (URISyntaxException e) {
+                        throw new IOException(e);
+                    }
+                }));
             }
         }
         try {

plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java

@@ -72,11 +72,6 @@ class GoogleCloudStorageBlobContainer extends AbstractBlobContainer {
         writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists);
     }
 
-    @Override
-    public void deleteBlob(String blobName) throws IOException {
-        blobStore.deleteBlob(buildKey(blobName));
-    }
-
     @Override
     public DeleteResult delete() throws IOException {
         return blobStore.deleteDirectory(path().buildAsString());

plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

@@ -314,19 +314,6 @@ class GoogleCloudStorageBlobStore implements BlobStore {
         }
     }
 
-    /**
-     * Deletes the blob from the specific bucket
-     *
-     * @param blobName name of the blob
-     */
-    void deleteBlob(String blobName) throws IOException {
-        final BlobId blobId = BlobId.of(bucketName, blobName);
-        final boolean deleted = SocketAccess.doPrivilegedIOException(() -> client().delete(blobId));
-        if (deleted == false) {
-            throw new NoSuchFileException("Blob [" + blobName + "] does not exist");
-        }
-    }
-
     /**
      * Deletes the given path and all its children.
      *
@@ -403,5 +390,4 @@ class GoogleCloudStorageBlobStore implements BlobStore {
         assert s != null;
         return keyPath + s;
     }
-
 }

plugins/repository-hdfs/src/main/java/org/elasticsearch/repositories/hdfs/HdfsBlobContainer.java

@@ -43,6 +43,7 @@ import java.nio.file.NoSuchFileException;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 
 final class HdfsBlobContainer extends AbstractBlobContainer {
@@ -59,17 +60,6 @@ final class HdfsBlobContainer extends AbstractBlobContainer {
         this.bufferSize = bufferSize;
     }
 
-    @Override
-    public void deleteBlob(String blobName) throws IOException {
-        try {
-            if (store.execute(fileContext -> fileContext.delete(new Path(path, blobName), true)) == false) {
-                throw new NoSuchFileException("Blob [" + blobName + "] does not exist");
-            }
-        } catch (FileNotFoundException fnfe) {
-            throw new NoSuchFileException("[" + blobName + "] blob not found");
-        }
-    }
-
     // TODO: See if we can get precise result reporting.
     private static final DeleteResult DELETE_RESULT = new DeleteResult(1L, 0L);
 
@@ -79,6 +69,27 @@ final class HdfsBlobContainer extends AbstractBlobContainer {
         return DELETE_RESULT;
     }
 
+    @Override
+    public void deleteBlobsIgnoringIfNotExists(final List<String> blobNames) throws IOException {
+        IOException ioe = null;
+        for (String blobName : blobNames) {
+            try {
+                store.execute(fileContext -> fileContext.delete(new Path(path, blobName), true));
+            } catch (final FileNotFoundException ignored) {
+                // This exception is ignored
+            } catch (IOException e) {
+                if (ioe == null) {
+                    ioe = e;
+                } else {
+                    ioe.addSuppressed(e);
+                }
+            }
+        }
+        if (ioe != null) {
+            throw ioe;
+        }
+    }
+
     @Override
     public InputStream readBlob(String blobName) throws IOException {
         // FSDataInputStream does buffering internally

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

@@ -107,11 +107,6 @@ class S3BlobContainer extends AbstractBlobContainer {
         writeBlob(blobName, inputStream, blobSize, failIfAlreadyExists);
     }
 
-    @Override
-    public void deleteBlob(String blobName) throws IOException {
-        deleteBlobIgnoringIfNotExists(blobName);
-    }
-
     @Override
     public DeleteResult delete() throws IOException {
         final AtomicLong deletedBlobs = new AtomicLong();
@@ -215,16 +210,6 @@ class S3BlobContainer extends AbstractBlobContainer {
         return new DeleteObjectsRequest(bucket).withKeys(blobs.toArray(Strings.EMPTY_ARRAY)).withQuiet(true);
     }
 
-    @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);
-        }
-    }
-
     @Override
     public Map<String, BlobMetaData> listBlobsByPrefix(@Nullable String blobNamePrefix) throws IOException {
         try (AmazonS3Reference clientReference = blobStore.clientReference()) {

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java

@@ -71,11 +71,6 @@ public class S3BlobStoreContainerTests extends ESBlobStoreContainerTestCase {
         return randomMockS3BlobStore();
     }
 
-    @Override
-    public void testDeleteBlob() {
-        assumeFalse("not implemented because of S3's weak consistency model", true);
-    }
-
     @Override
     public void testVerifyOverwriteFails() {
         assumeFalse("not implemented because of S3's weak consistency model", true);

server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java

@@ -89,17 +89,6 @@ public interface BlobContainer {
      */
     void writeBlobAtomic(String blobName, InputStream inputStream, long blobSize, boolean failIfAlreadyExists) throws IOException;
 
-    /**
-     * Deletes the blob with the given name, if the blob exists. If the blob does not exist,
-     * this method may throw a {@link NoSuchFileException} if the underlying implementation supports an existence check before delete.
-     *
-     * @param   blobName
-     *          The name of the blob to delete.
-     * @throws  NoSuchFileException if the blob does not exist
-     * @throws  IOException if the blob exists but could not be deleted.
-     */
-    void deleteBlob(String blobName) throws IOException;
-
     /**
      * Deletes this container and all its contents from the repository.
      *
@@ -109,44 +98,13 @@ public interface BlobContainer {
     DeleteResult delete() throws IOException;
 
     /**
-     * Deletes the blobs with given names. Unlike {@link #deleteBlob(String)} this method will not throw an exception
+     * Deletes the blobs with given names. This method will not throw an exception
      * when one or multiple of the given blobs don't exist and simply ignore this case.
      *
      * @param   blobNames  The names of the blob to delete.
      * @throws  IOException if a subset of blob exists but could not be deleted.
      */
-    default void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOException {
+    void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOException;
-        IOException ioe = null;
-        for (String blobName : blobNames) {
-            try {
-                deleteBlobIgnoringIfNotExists(blobName);
-            } catch (IOException e) {
-                if (ioe == null) {
-                    ioe = e;
-                } else {
-                    ioe.addSuppressed(e);
-                }
-            }
-        }
-        if (ioe != null) {
-            throw ioe;
-        }
-    }
-
-    /**
-     * Deletes a blob with giving name, ignoring if the blob does not exist.
-     *
-     * @param   blobName
-     *          The name of the blob to delete.
-     * @throws  IOException if the blob exists but could not be deleted.
-     */
-    default void deleteBlobIgnoringIfNotExists(String blobName) throws IOException {
-        try {
-            deleteBlob(blobName);
-        } catch (final NoSuchFileException ignored) {
-            // This exception is ignored
-        }
-    }
 
     /**
      * Lists all blobs in the container.

server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java

@@ -44,7 +44,9 @@ import java.nio.file.SimpleFileVisitor;
 import java.nio.file.StandardCopyOption;
 import java.nio.file.StandardOpenOption;
 import java.nio.file.attribute.BasicFileAttributes;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicLong;
 
@@ -112,24 +114,6 @@ public class FsBlobContainer extends AbstractBlobContainer {
         return unmodifiableMap(builder);
     }
 
-    @Override
-    public void deleteBlob(String blobName) throws IOException {
-        Path blobPath = path.resolve(blobName);
-        if (Files.isDirectory(blobPath)) {
-            // delete directory recursively as long as it is empty (only contains empty directories),
-            // which is the reason we aren't deleting any files, only the directories on the post-visit
-            Files.walkFileTree(blobPath, new SimpleFileVisitor<Path>() {
-                @Override
-                public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
-                    Files.delete(dir);
-                    return FileVisitResult.CONTINUE;
-                }
-            });
-        } else {
-            Files.delete(blobPath);
-        }
-    }
-
     @Override
     public DeleteResult delete() throws IOException {
         final AtomicLong filesDeleted = new AtomicLong(0L);
@@ -153,6 +137,11 @@ public class FsBlobContainer extends AbstractBlobContainer {
         return new DeleteResult(filesDeleted.get(), bytesDeleted.get());
     }
 
+    @Override
+    public void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOException {
+        IOUtils.rm(blobNames.stream().map(path::resolve).toArray(Path[]::new));
+    }
+
     @Override
     public InputStream readBlob(String name) throws IOException {
         final Path resolvedPath = path.resolve(name);
@@ -166,7 +155,7 @@ public class FsBlobContainer extends AbstractBlobContainer {
     @Override
     public void writeBlob(String blobName, InputStream inputStream, long blobSize, boolean failIfAlreadyExists) throws IOException {
         if (failIfAlreadyExists == false) {
-            deleteBlobIgnoringIfNotExists(blobName);
+            deleteBlobsIgnoringIfNotExists(Collections.singletonList(blobName));
         }
         final Path file = path.resolve(blobName);
         try (OutputStream outputStream = Files.newOutputStream(file, StandardOpenOption.CREATE_NEW)) {
@@ -189,7 +178,7 @@ public class FsBlobContainer extends AbstractBlobContainer {
             moveBlobAtomic(tempBlob, blobName, failIfAlreadyExists);
         } catch (IOException ex) {
             try {
-                deleteBlobIgnoringIfNotExists(tempBlob);
+                deleteBlobsIgnoringIfNotExists(Collections.singletonList(tempBlob));
             } catch (IOException e) {
                 ex.addSuppressed(e);
             }
@@ -209,7 +198,7 @@ public class FsBlobContainer extends AbstractBlobContainer {
             if (failIfAlreadyExists) {
                 throw new FileAlreadyExistsException("blob [" + targetBlobPath + "] already exists, cannot overwrite");
             } else {
-                deleteBlobIgnoringIfNotExists(targetBlobName);
+                deleteBlobsIgnoringIfNotExists(Collections.singletonList(targetBlobName));
             }
         }
         Files.move(sourceBlobPath, targetBlobPath, StandardCopyOption.ATOMIC_MOVE);

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -979,9 +979,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
         if (isReadOnly() == false) {
             try {
                 final String testPrefix = testBlobPrefix(seed);
-                final BlobContainer container = blobStore().blobContainer(basePath().add(testPrefix));
+                blobStore().blobContainer(basePath().add(testPrefix)).delete();
-                container.deleteBlobsIgnoringIfNotExists(new ArrayList<>(container.listBlobs().keySet()));
-                blobStore().blobContainer(basePath()).deleteBlobIgnoringIfNotExists(testPrefix);
             } catch (IOException exp) {
                 throw new RepositoryVerificationException(metadata.name(), "cannot delete test data at " + basePath(), exp);
             }

server/src/main/java/org/elasticsearch/repositories/blobstore/ChecksumBlobStoreFormat.java

@@ -114,13 +114,6 @@ public final class ChecksumBlobStoreFormat<T extends ToXContent> {
         return readBlob(blobContainer, blobName);
     }
 
-    /**
-     * Deletes obj in the blob container
-     */
-    public void delete(BlobContainer blobContainer, String name) throws IOException {
-        blobContainer.deleteBlob(blobName(name));
-    }
-
     public String blobName(String name) {
         return String.format(Locale.ROOT, blobNameFormat, name);
     }

server/src/test/java/org/elasticsearch/snapshots/mockstore/MockEventuallyConsistentRepository.java

@@ -225,10 +225,12 @@ public class MockEventuallyConsistentRepository extends BlobStoreRepository {
             }
 
             @Override
-            public void deleteBlob(String blobName) {
+            public void deleteBlobsIgnoringIfNotExists(List<String> blobNames) {
                 ensureNotClosed();
                 synchronized (context.actions) {
-                    context.actions.add(new BlobStoreAction(Operation.DELETE, path.buildAsString() + blobName));
+                    for (String blobName : blobNames) {
+                        context.actions.add(new BlobStoreAction(Operation.DELETE, path.buildAsString() + blobName));
+                    }
                 }
             }
 

server/src/test/java/org/elasticsearch/snapshots/mockstore/MockEventuallyConsistentRepositoryTests.java

@@ -91,7 +91,7 @@ public class MockEventuallyConsistentRepositoryTests extends ESTestCase {
             final int lengthWritten = randomIntBetween(1, 100);
             final byte[] blobData = randomByteArrayOfLength(lengthWritten);
             blobContainer.writeBlob(blobName, new ByteArrayInputStream(blobData), lengthWritten, true);
-            blobContainer.deleteBlob(blobName);
+            blobContainer.deleteBlobsIgnoringIfNotExists(Collections.singletonList(blobName));
             assertThrowsOnInconsistentRead(blobContainer, blobName);
             blobStoreContext.forceConsistent();
             expectThrows(NoSuchFileException.class, () -> blobContainer.readBlob(blobName));

test/framework/src/main/java/org/elasticsearch/repositories/ESBlobStoreContainerTestCase.java

@@ -33,6 +33,7 @@ import java.io.InputStream;
 import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.NoSuchFileException;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -120,22 +121,6 @@ public abstract class ESBlobStoreContainerTestCase extends ESTestCase {
         }
     }
 
-    public void testDeleteBlob() throws IOException {
-        try (BlobStore store = newBlobStore()) {
-            final String blobName = "foobar";
-            final BlobContainer container = store.blobContainer(new BlobPath());
-            expectThrows(NoSuchFileException.class, () -> container.deleteBlob(blobName));
-
-            byte[] data = randomBytes(randomIntBetween(10, scaledRandomIntBetween(1024, 1 << 16)));
-            final BytesArray bytesArray = new BytesArray(data);
-            writeBlob(container, blobName, bytesArray, randomBoolean());
-            container.deleteBlob(blobName); // should not raise
-
-            // blob deleted, so should raise again
-            expectThrows(NoSuchFileException.class, () -> container.deleteBlob(blobName));
-        }
-    }
-
     public void testDeleteBlobs() throws IOException {
         try (BlobStore store = newBlobStore()) {
             final List<String> blobNames = Arrays.asList("foobar", "barfoo");
@@ -153,18 +138,6 @@ public abstract class ESBlobStoreContainerTestCase extends ESTestCase {
         }
     }
 
-    public void testDeleteBlobIgnoringIfNotExists() throws IOException {
-        try (BlobStore store = newBlobStore()) {
-            BlobPath blobPath = new BlobPath();
-            if (randomBoolean()) {
-                blobPath = blobPath.add(randomAlphaOfLengthBetween(1, 10));
-            }
-
-            final BlobContainer container = store.blobContainer(blobPath);
-            container.deleteBlobIgnoringIfNotExists("does_not_exist");
-        }
-    }
-
     public void testVerifyOverwriteFails() throws IOException {
         try (BlobStore store = newBlobStore()) {
             final String blobName = "foobar";
@@ -174,7 +147,7 @@ public abstract class ESBlobStoreContainerTestCase extends ESTestCase {
             writeBlob(container, blobName, bytesArray, true);
             // should not be able to overwrite existing blob
             expectThrows(FileAlreadyExistsException.class, () -> writeBlob(container, blobName, bytesArray, true));
-            container.deleteBlob(blobName);
+            container.deleteBlobsIgnoringIfNotExists(Collections.singletonList(blobName));
             writeBlob(container, blobName, bytesArray, true); // after deleting the previous blob, we should be able to write to it again
         }
     }

test/framework/src/main/java/org/elasticsearch/snapshots/mockstore/BlobContainerWrapper.java

@@ -25,6 +25,7 @@ import org.elasticsearch.common.blobstore.DeleteResult;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.List;
 import java.util.Map;
 
 public class BlobContainerWrapper implements BlobContainer {
@@ -55,19 +56,14 @@ public class BlobContainerWrapper implements BlobContainer {
         delegate.writeBlobAtomic(blobName, inputStream, blobSize, failIfAlreadyExists);
     }
 
-    @Override
-    public void deleteBlob(String blobName) throws IOException {
-        delegate.deleteBlob(blobName);
-    }
-
     @Override
     public DeleteResult delete() throws IOException {
         return delegate.delete();
     }
 
     @Override
-    public void deleteBlobIgnoringIfNotExists(final String blobName) throws IOException {
+    public void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOException {
-        delegate.deleteBlobIgnoringIfNotExists(blobName);
+        delegate.deleteBlobsIgnoringIfNotExists(blobNames);
     }
 
     @Override

test/framework/src/main/java/org/elasticsearch/snapshots/mockstore/MockRepository.java

@@ -312,18 +312,6 @@ public class MockRepository extends FsRepository {
                 return super.readBlob(name);
             }
 
-            @Override
-            public void deleteBlob(String blobName) throws IOException {
-                maybeIOExceptionOrBlock(blobName);
-                super.deleteBlob(blobName);
-            }
-
-            @Override
-            public void deleteBlobIgnoringIfNotExists(String blobName) throws IOException {
-                maybeIOExceptionOrBlock(blobName);
-                super.deleteBlobIgnoringIfNotExists(blobName);
-            }
-
             @Override
             public DeleteResult delete() throws IOException {
                 DeleteResult deleteResult = DeleteResult.ZERO;
@@ -334,10 +322,12 @@ public class MockRepository extends FsRepository {
                 long deleteBlobCount = blobs.size();
                 long deleteByteCount = 0L;
                 for (String blob : blobs.values().stream().map(BlobMetaData::name).collect(Collectors.toList())) {
-                    deleteBlobIgnoringIfNotExists(blob);
+                    maybeIOExceptionOrBlock(blob);
+                    deleteBlobsIgnoringIfNotExists(Collections.singletonList(blob));
                     deleteByteCount += blobs.get(blob).length();
                 }
-                blobStore().blobContainer(path().parent()).deleteBlob(path().toArray()[path().toArray().length - 1]);
+                blobStore().blobContainer(path().parent()).deleteBlobsIgnoringIfNotExists(
+                    Collections.singletonList(path().toArray()[path().toArray().length - 1]));
                 return deleteResult.add(deleteBlobCount, deleteByteCount);
             }