Clarify the semantics of the BlobContainer interface

This commit clarifies the behavior that must be adhered to by any
implementors of the BlobContainer interface.  This is done through
expanded Javadocs.

Closes #18157
Closes #15580

Author: Ali Beyad

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

@@ -27,60 +27,138 @@
 import java.util.Map;
 
 /**
- *
+ * An interface for managing a repository of blob entries, where each blob entry is just a named group of bytes.
  */
 public interface BlobContainer {
 
+    /**
+     * Gets the {@link BlobPath} that defines the implementation specific paths to where the blobs are contained.
+     *
+     * @return  the BlobPath where the blobs are contained
+     */
     BlobPath path();
 
+    /**
+     * Tests whether a blob with the given blob name exists in the container.
+     *
+     * @param   blobName
+     *          The name of the blob whose existence is to be determined.
+     * @return  {@code true} if a blob exists in the {@link BlobContainer} with the given name, and {@code false} otherwise.
+     * @throws  IOException if any error occurred while attempting to ascertain if the blob exists
+     */
     boolean blobExists(String blobName);
 
     /**
-     * Creates a new InputStream for the given blob name
+     * Creates a new {@link InputStream} for the given blob name.
+     *
+     * @param   blobName
+     *          The name of the blob to get an {@link InputStream} for.
+     * @return  The {@code InputStream} to read the blob.
+     * @throws  IOException if the blob does not exist or can not be read.
      */
     InputStream readBlob(String blobName) throws IOException;
 
     /**
-     * Reads blob content from the input stream and writes it to the blob store
+     * Reads blob content from the input stream and writes it to the container in a new blob with the given name.
+     * This method assumes the container does not already contain a blob of the same blobName.  If a blob by the
+     * same name already exists, the operation will fail and an {@link IOException} will be thrown.
+     *
+     * @param   blobName
+     *          The name of the blob to write the contents of the input stream to.
+     * @param   inputStream
+     *          The input stream from which to retrieve the bytes to write to the blob.
+     * @param   blobSize
+     *          The size of the blob to be written, in bytes.  It is implementation dependent whether
+     *          this value is used in writing the blob to the repository.
+     * @throws  IOException if the input stream could not be read, a blob by the same name already exists,
+     *          or the target blob could not be written to.
      */
     void writeBlob(String blobName, InputStream inputStream, long blobSize) throws IOException;
 
     /**
-     * Writes bytes to the blob
+     * Writes the input bytes to a new blob in the container with the given name.  This method assumes the
+     * container does not already contain a blob of the same blobName.  If a blob by the same name already
+     * exists, the operation will fail and an {@link IOException} will be thrown.
+     *
+     * TODO: Remove this in favor of a single {@link #writeBlob(String, InputStream, long)} method.
+     *       See https://github.com/elastic/elasticsearch/issues/18528
+     *
+     * @param   blobName
+     *          The name of the blob to write the contents of the input stream to.
+     * @param   bytes
+     *          The bytes to write to the blob.
+     * @throws  IOException if a blob by the same name already exists, or the target blob could not be written to.
      */
     void writeBlob(String blobName, BytesReference bytes) throws IOException;
 
     /**
-     * Deletes a blob with giving name.
+     * Deletes a blob with giving name, if the blob exists.  If the blob does not exist, this method throws an IOException.
      *
-     * If a blob exists but cannot be deleted an exception has to be thrown.
+     * @param   blobName
+     *          The name of the blob to delete.
+     * @throws  IOException if the blob does not exist, or if the blob exists but could not be deleted.
      */
     void deleteBlob(String blobName) throws IOException;
 
     /**
-     * Deletes blobs with giving names.
+     * Deletes blobs with the given names.  If any subset of the names do not exist in the container, this method has no
+     * effect for those names, and will delete the blobs for those names that do exist.  If any of the blobs failed
+     * to delete, those blobs that were processed before it and successfully deleted will remain deleted.  An exception
+     * is thrown at the first blob entry that fails to delete (TODO: is this the right behavior?  Should we collect
+     * all the failed deletes into a single IOException instead?)
      *
-     * If a blob exists but cannot be deleted an exception has to be thrown.
+     * TODO: remove, see https://github.com/elastic/elasticsearch/issues/18529
+     *
+     * @param   blobNames
+     *          The collection of blob names to delete from the container.
+     * @throws  IOException if any of the blobs in the collection exists but could not be deleted.
      */
     void deleteBlobs(Collection<String> blobNames) throws IOException;
 
     /**
-     * Deletes all blobs in the container that match the specified prefix.
+     * Deletes all blobs in the container that match the specified prefix.  If any of the blobs failed to delete,
+     * those blobs that were processed before it and successfully deleted will remain deleted.  An exception is
+     * thrown at the first blob entry that fails to delete (TODO: is this the right behavior?  Should we collect
+     * all the failed deletes into a single IOException instead?)
+     *
+     * TODO: remove, see: https://github.com/elastic/elasticsearch/issues/18529
+     *
+     * @param   blobNamePrefix
+     *          The prefix to match against blob names in the container.  Any blob whose name has the prefix will be deleted.
+     * @throws  IOException if any of the matching blobs failed to delete.
      */
     void deleteBlobsByPrefix(String blobNamePrefix) throws IOException;
 
     /**
-     * Lists all blobs in the container
+     * Lists all blobs in the container.
+     *
+     * @return  A map of all the blobs in the container.  The keys in the map are the names of the blobs and
+     *          the values are {@link BlobMetaData}, containing basic information about each blob.
+     * @throws  IOException if there were any failures in reading from the blob container.
      */
     Map<String, BlobMetaData> listBlobs() throws IOException;
 
     /**
-     * Lists all blobs in the container that match specified prefix
+     * Lists all blobs in the container that match the specified prefix.
+     *
+     * @param   blobNamePrefix
+     *          The prefix to match against blob names in the container.
+     * @return  A map of the matching blobs in the container.  The keys in the map are the names of the blobs
+     *          and the values are {@link BlobMetaData}, containing basic information about each blob.
+     * @throws  IOException if there were any failures in reading from the blob container.
      */
     Map<String, BlobMetaData> listBlobsByPrefix(String blobNamePrefix) throws IOException;
 
     /**
-     * Atomically renames source blob into target blob
+     * Atomically renames the source blob into the target blob.  If the source blob does not exist or the
+     * target blob already exists, an exception is thrown.
+     *
+     * @param   sourceBlobName
+     *          The blob to rename.
+     * @param   targetBlobName
+     *          The name of the blob after the renaming.
+     * @throws  IOException if the source blob does not exist, the target blob already exists,
+     *          or there were any failures in reading from the blob container.
      */
     void move(String sourceBlobName, String targetBlobName) throws IOException;
 }

core/src/main/java/org/elasticsearch/common/blobstore/BlobMetaData.java

@@ -20,11 +20,17 @@
 package org.elasticsearch.common.blobstore;
 
 /**
- *
+ * An interface for providing basic metadata about a blob.
  */
 public interface BlobMetaData {
 
+    /**
+     * Gets the name of the blob.
+     */
     String name();
 
+    /**
+     * Gets the size of the blob in bytes.
+     */
     long length();
 }

core/src/main/java/org/elasticsearch/common/blobstore/BlobPath.java

@@ -19,14 +19,13 @@
 
 package org.elasticsearch.common.blobstore;
 
-
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 
 /**
- *
+ * The list of paths where a blob can reside.  The contents of the paths are dependent upon the implementation of {@link BlobContainer}.
  */
 public class BlobPath implements Iterable<String> {
 

core/src/main/java/org/elasticsearch/common/blobstore/BlobStore.java

@@ -22,12 +22,18 @@
 import java.io.IOException;
 
 /**
- *
+ * An interface for storing blobs.
  */
 public interface BlobStore extends Closeable {
 
+    /**
+     * Get a blob container instance for storing blobs at the given {@link BlobPath}.
+     */
     BlobContainer blobContainer(BlobPath path);
 
+    /**
+     * Delete the blob store at the given {@link BlobPath}.
+     */
     void delete(BlobPath path) throws IOException;
 
 }

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

@@ -41,7 +41,12 @@
 import static java.util.Collections.unmodifiableMap;
 
 /**
+ * A file system based implementation of {@link org.elasticsearch.common.blobstore.BlobContainer}.
+ * All blobs in the container are stored on a file system, the location of which is specified by the {@link BlobPath}.
  *
+ * Note that the methods in this implementation of {@link org.elasticsearch.common.blobstore.BlobContainer} may
+ * additionally throw a {@link java.lang.SecurityException} if the configured {@link java.lang.SecurityManager}
+ * does not permit read and/or write access to the underlying files.
  */
 public class FsBlobContainer extends AbstractBlobContainer {
 

core/src/main/java/org/elasticsearch/common/blobstore/support/AbstractBlobContainer.java

@@ -30,7 +30,7 @@
 import java.util.Map;
 
 /**
- *
+ * A base abstract blob container that implements higher level container methods.
  */
 public abstract class AbstractBlobContainer implements BlobContainer {
 
@@ -55,11 +55,11 @@ public void deleteBlobsByPrefix(final String blobNamePrefix) throws IOException
 
     @Override
     public void deleteBlobs(Collection<String> blobNames) throws IOException {
-        for(String blob: blobNames) {
+        for (String blob: blobNames) {
             deleteBlob(blob);
         }
     }
-    
+
     @Override
     public void writeBlob(String blobName, BytesReference bytes) throws IOException {
         try (InputStream stream = bytes.streamInput()) {

core/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java

@@ -283,6 +283,7 @@ protected void randomCorruption(BlobContainer blobContainer, String blobName) th
             int location = randomIntBetween(0, buffer.length - 1);
             buffer[location] = (byte) (buffer[location] ^ 42);
         } while (originalChecksum == checksum(buffer));
+        blobContainer.deleteBlob(blobName); // delete original before writing new blob
         blobContainer.writeBlob(blobName, new BytesArray(buffer));
     }
 

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

@@ -154,4 +154,4 @@ public boolean accept(Path path) {
     public Map<String, BlobMetaData> listBlobs() throws IOException {
         return listBlobsByPrefix(null);
     }
-}
+}

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

@@ -112,5 +112,19 @@ public void testMoveAndList() throws IOException {
         }
     }
 
+    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/15579")
+    public void testOverwriteFails() throws IOException {
+        try (final BlobStore store = newBlobStore()) {
+            final String blobName = "foobar";
+            final BlobContainer container = store.blobContainer(new BlobPath());
+            byte[] data = randomBytes(randomIntBetween(10, scaledRandomIntBetween(1024, 1 << 16)));
+            final BytesArray bytesArray = new BytesArray(data);
+            container.writeBlob(blobName, bytesArray);
+            expectThrows(IOException.class, () -> container.writeBlob(blobName, bytesArray));
+            container.deleteBlob(blobName);
+            container.writeBlob(blobName, bytesArray); // deleted it, so should be able to write it again
+        }
+    }
+
     protected abstract BlobStore newBlobStore() throws IOException;
 }