Remove blobExists Method from BlobContainer (#44472)

original-brownbear · web-flow · commit 4b8fd4e76f1e · 2019-07-17T09:15:22.000+02:00
* We only use this method in one place in production code and can replace that with a read -&gt; remove it to simplify the interface
   * Keep it as an implementation detail in the Azure repository
diff --git a/modules/repository-url/src/main/java/org/elasticsearch/common/blobstore/url/URLBlobContainer.java b/modules/repository-url/src/main/java/org/elasticsearch/common/blobstore/url/URLBlobContainer.java
@@ -101,14 +101,6 @@ public void delete() {
         throw new UnsupportedOperationException("URL repository is read only");
     }
 
-    /**
-     * This operation is not supported by URLBlobContainer
-     */
-    @Override
-    public boolean blobExists(String blobName) {
-        throw new UnsupportedOperationException("URL repository doesn't support this operation");
-    }
-
     @Override
     public InputStream readBlob(String name) throws IOException {
         try {
diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobContainer.java b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobContainer.java
@@ -57,8 +57,7 @@ public class AzureBlobContainer extends AbstractBlobContainer {
         this.threadPool = threadPool;
     }
 
-    @Override
-    public boolean blobExists(String blobName) {
+    private boolean blobExists(String blobName) {
         logger.trace("blobExists({})", blobName);
         try {
             return blobStore.blobExists(buildKey(blobName));
diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java
@@ -22,7 +22,6 @@
 import org.elasticsearch.common.blobstore.BlobContainer;
 import org.elasticsearch.common.blobstore.BlobMetaData;
 import org.elasticsearch.common.blobstore.BlobPath;
-import org.elasticsearch.common.blobstore.BlobStoreException;
 import org.elasticsearch.common.blobstore.support.AbstractBlobContainer;
 
 import java.io.IOException;
@@ -42,15 +41,6 @@ class GoogleCloudStorageBlobContainer extends AbstractBlobContainer {
         this.path = path.buildAsString();
     }
 
-    @Override
-    public boolean blobExists(String blobName) {
-        try {
-            return blobStore.blobExists(buildKey(blobName));
-        } catch (Exception e) {
-            throw new BlobStoreException("Failed to check if blob [" + blobName + "] exists", e);
-        }
-    }
-
     @Override
     public Map<String, BlobMetaData> listBlobs() throws IOException {
         return blobStore.listBlobs(path);
diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java
@@ -163,18 +163,6 @@ Map<String, BlobContainer> listChildren(BlobPath path) throws IOException {
         return mapBuilder.immutableMap();
     }
 
-    /**
-     * Returns true if the blob exists in the specific bucket
-     *
-     * @param blobName name of the blob
-     * @return true iff the blob exists
-     */
-    boolean blobExists(String blobName) throws IOException {
-        final BlobId blobId = BlobId.of(bucketName, blobName);
-        final Blob blob = SocketAccess.doPrivilegedIOException(() -> client().get(blobId));
-        return blob != null;
-    }
-
     /**
      * Returns an {@link java.io.InputStream} for the given blob name
      *
diff --git a/plugins/repository-hdfs/src/main/java/org/elasticsearch/repositories/hdfs/HdfsBlobContainer.java b/plugins/repository-hdfs/src/main/java/org/elasticsearch/repositories/hdfs/HdfsBlobContainer.java
@@ -58,15 +58,6 @@ final class HdfsBlobContainer extends AbstractBlobContainer {
         this.bufferSize = bufferSize;
     }
 
-    @Override
-    public boolean blobExists(String blobName) {
-        try {
-            return store.execute(fileContext -> fileContext.util().exists(new Path(path, blobName)));
-        } catch (Exception e) {
-            return false;
-        }
-    }
-
     @Override
     public void deleteBlob(String blobName) throws IOException {
         try {
diff --git a/plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsBlobStoreContainerTests.java b/plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsBlobStoreContainerTests.java
@@ -31,6 +31,7 @@
 import org.elasticsearch.common.blobstore.BlobStore;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.repositories.ESBlobStoreContainerTestCase;
+import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
 
 import javax.security.auth.Subject;
 import java.io.IOException;
@@ -137,6 +138,6 @@ public void testReadOnly() throws Exception {
         byte[] data = randomBytes(randomIntBetween(10, scaledRandomIntBetween(1024, 1 << 16)));
         writeBlob(container, "foo", new BytesArray(data), randomBoolean());
         assertArrayEquals(readBlobFully(container, "foo", data.length), data);
-        assertTrue(container.blobExists("foo"));
+        assertTrue(BlobStoreTestUtil.blobExists(container, "foo"));
     }
 }
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
@@ -42,7 +42,6 @@
 import org.elasticsearch.common.blobstore.BlobContainer;
 import org.elasticsearch.common.blobstore.BlobMetaData;
 import org.elasticsearch.common.blobstore.BlobPath;
-import org.elasticsearch.common.blobstore.BlobStoreException;
 import org.elasticsearch.common.blobstore.support.AbstractBlobContainer;
 import org.elasticsearch.common.blobstore.support.PlainBlobMetaData;
 import org.elasticsearch.common.collect.Tuple;
@@ -79,15 +78,6 @@ class S3BlobContainer extends AbstractBlobContainer {
         this.keyPath = path.buildAsString();
     }
 
-    @Override
-    public boolean blobExists(String blobName) {
-        try (AmazonS3Reference clientReference = blobStore.clientReference()) {
-            return SocketAccess.doPrivileged(() -> clientReference.client().doesObjectExist(blobStore.bucket(), buildKey(blobName)));
-        } catch (final Exception e) {
-            throw new BlobStoreException("Failed to check if blob [" + blobName +"] exists", e);
-        }
-    }
-
     @Override
     public InputStream readBlob(String blobName) throws IOException {
         try (AmazonS3Reference clientReference = blobStore.clientReference()) {
diff --git a/server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java b/server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java
@@ -38,15 +38,6 @@ public interface BlobContainer {
      */
     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.
-     */
-    boolean blobExists(String blobName);
-
     /**
      * Creates a new {@link InputStream} for the given blob name.
      *
diff --git a/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java b/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java
@@ -127,11 +127,6 @@ public void delete() throws IOException {
         IOUtils.rm(path);
     }
 
-    @Override
-    public boolean blobExists(String blobName) {
-        return Files.exists(path.resolve(blobName));
-    }
-
     @Override
     public InputStream readBlob(String name) throws IOException {
         final Path resolvedPath = path.resolve(name);
diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
@@ -1033,7 +1033,7 @@ public void verify(String seed, DiscoveryNode localNode) {
             }
         } else {
             BlobContainer testBlobContainer = blobStore().blobContainer(basePath().add(testBlobPrefix(seed)));
-            if (testBlobContainer.blobExists("master.dat")) {
+            try (InputStream ignored = testBlobContainer.readBlob("master.dat")) {
                 try {
                     BytesArray bytes = new BytesArray(seed);
                     try (InputStream stream = bytes.streamInput()) {
@@ -1043,11 +1043,13 @@ public void verify(String seed, DiscoveryNode localNode) {
                     throw new RepositoryVerificationException(metadata.name(), "store location [" + blobStore() +
                         "] is not accessible on the node [" + localNode + "]", exp);
                 }
-            } else {
+            } catch (NoSuchFileException e) {
                 throw new RepositoryVerificationException(metadata.name(), "a file written by master to the store [" + blobStore() +
                     "] cannot be accessed on the node [" + localNode + "]. " +
                     "This might indicate that the store [" + blobStore() + "] is not shared between this node and the master node or " +
-                    "that permissions on the store don't allow reading files written by the master node");
+                    "that permissions on the store don't allow reading files written by the master node", e);
+            } catch (IOException e) {
+                throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e);
             }
         }
     }
diff --git a/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreTests.java b/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreTests.java
@@ -27,6 +27,7 @@
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.repositories.ESBlobStoreTestCase;
+import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
 
 import java.io.IOException;
 import java.nio.file.Files;
@@ -74,7 +75,7 @@ public void testReadOnly() throws Exception {
             byte[] data = randomBytes(randomIntBetween(10, scaledRandomIntBetween(1024, 1 << 16)));
             writeBlob(container, "test", new BytesArray(data));
             assertArrayEquals(readBlobFully(container, "test", data.length), data);
-            assertTrue(container.blobExists("test"));
+            assertTrue(BlobStoreTestUtil.blobExists(container, "test"));
         }
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java b/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java
@@ -36,6 +36,7 @@
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.translog.BufferedChecksumStreamOutput;
+import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
 import org.elasticsearch.repositories.blobstore.ChecksumBlobStoreFormat;
 import org.elasticsearch.snapshots.mockstore.BlobContainerWrapper;
 
@@ -193,11 +194,12 @@ public Void call() throws Exception {
                     return null;
                 }
             });
+            // signalling
             block.await(5, TimeUnit.SECONDS);
-            assertFalse(blobContainer.blobExists("test-blob"));
+            assertFalse(BlobStoreTestUtil.blobExists(blobContainer, "test-blob"));
             unblock.countDown();
             future.get();
-            assertTrue(blobContainer.blobExists("test-blob"));
+            assertTrue(BlobStoreTestUtil.blobExists(blobContainer, "test-blob"));
         } finally {
             threadPool.shutdown();
         }
diff --git a/server/src/test/java/org/elasticsearch/snapshots/mockstore/BlobContainerWrapper.java b/server/src/test/java/org/elasticsearch/snapshots/mockstore/BlobContainerWrapper.java
@@ -38,11 +38,6 @@ public BlobPath path() {
         return delegate.path();
     }
 
-    @Override
-    public boolean blobExists(String blobName) {
-        return delegate.blobExists(blobName);
-    }
-
     @Override
     public InputStream readBlob(String name) throws IOException {
         return delegate.readBlob(name);
diff --git a/server/src/test/java/org/elasticsearch/snapshots/mockstore/MockRepository.java b/server/src/test/java/org/elasticsearch/snapshots/mockstore/MockRepository.java
@@ -311,11 +311,6 @@ private void blockExecutionAndFail(final String blobName) throws IOException {
                 super(delegate);
             }
 
-            @Override
-            public boolean blobExists(String blobName) {
-                return super.blobExists(blobName);
-            }
-
             @Override
             public InputStream readBlob(String name) throws IOException {
                 maybeIOExceptionOrBlock(name);
diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/AbstractThirdPartyRepositoryTestCase.java b/test/framework/src/main/java/org/elasticsearch/repositories/AbstractThirdPartyRepositoryTestCase.java
@@ -261,9 +261,9 @@ protected void doRun() throws Exception {
                 final BlobStore blobStore = repo.blobStore();
                 future.onResponse(
                     blobStore.blobContainer(BlobPath.cleanPath().add("indices")).children().containsKey("foo")
-                        && blobStore.blobContainer(BlobPath.cleanPath().add("indices").add("foo")).blobExists("bar")
-                        && blobStore.blobContainer(BlobPath.cleanPath()).blobExists("meta-foo.dat")
-                        && blobStore.blobContainer(BlobPath.cleanPath()).blobExists("snap-foo.dat")
+                        && BlobStoreTestUtil.blobExists(blobStore.blobContainer(BlobPath.cleanPath().add("indices").add("foo")), "bar")
+                        && BlobStoreTestUtil.blobExists(blobStore.blobContainer(BlobPath.cleanPath()), "meta-foo.dat")
+                        && BlobStoreTestUtil.blobExists(blobStore.blobContainer(BlobPath.cleanPath()), "snap-foo.dat")
                 );
             }
         });
diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/ESBlobStoreTestCase.java b/test/framework/src/main/java/org/elasticsearch/repositories/ESBlobStoreTestCase.java
@@ -22,6 +22,7 @@
 import org.elasticsearch.common.blobstore.BlobPath;
 import org.elasticsearch.common.blobstore.BlobStore;
 import org.elasticsearch.common.bytes.BytesArray;
+import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
@@ -47,8 +48,8 @@ public void testContainerCreationAndDeletion() throws IOException {
             assertArrayEquals(readBlobFully(containerFoo, "test", data1.length), data1);
             assertArrayEquals(readBlobFully(containerBar, "test", data2.length), data2);
 
-            assertTrue(containerFoo.blobExists("test"));
-            assertTrue(containerBar.blobExists("test"));
+            assertTrue(BlobStoreTestUtil.blobExists(containerFoo, "test"));
+            assertTrue(BlobStoreTestUtil.blobExists(containerBar, "test"));
         }
     }
 
diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java
@@ -38,6 +38,7 @@
 import java.io.DataInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.NoSuchFileException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -60,6 +61,14 @@ public static void assertRepoConsistency(InternalTestCluster testCluster, String
         BlobStoreTestUtil.assertConsistency(repo, repo.threadPool().executor(ThreadPool.Names.GENERIC));
     }
 
+    public static boolean blobExists(BlobContainer container, String blobName) throws IOException {
+        try (InputStream ignored = container.readBlob(blobName)) {
+            return true;
+        } catch (NoSuchFileException e) {
+            return false;
+        }
+    }
+
     /**
      * Assert that there are no unreferenced indices or unreferenced root-level metadata blobs in any repository.
      * TODO: Expand the logic here to also check for unreferenced segment blobs and shard level metadata
@@ -74,11 +83,11 @@ public static void assertConsistency(BlobStoreRepository repository, Executor ex
             @Override
             protected void doRun() throws Exception {
                 final BlobContainer blobContainer = repository.blobContainer();
-                assertTrue(
-                    "Could not find index.latest blob for repo [" + repository + "]", blobContainer.blobExists("index.latest"));
                 final long latestGen;
                 try (DataInputStream inputStream = new DataInputStream(blobContainer.readBlob("index.latest"))) {
                     latestGen = inputStream.readLong();
+                } catch (NoSuchFileException e) {
+                    throw new AssertionError("Could not find index.latest blob for repo [" + repository + "]");
                 }
                 assertIndexGenerations(blobContainer, latestGen);
                 final RepositoryData repositoryData;
diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESBlobStoreRepositoryIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/ESBlobStoreRepositoryIntegTestCase.java
@@ -269,7 +269,7 @@ public void testIndicesDeletedFromRepository() throws Exception {
         latch.await();
         for (IndexId indexId : repositoryData.get().getIndices().values()) {
             if (indexId.getName().equals("test-idx-3")) {
-                assertFalse(indicesBlobContainer.get().blobExists(indexId.getId())); // deleted index
+                assertFalse(BlobStoreTestUtil.blobExists(indicesBlobContainer.get(), indexId.getId())); // deleted index
             }
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -57,8 +57,7 @@ public class AzureBlobContainer extends AbstractBlobContainer {`
`57`	`57`	`this.threadPool = threadPool;`
`58`	`58`	`}`
`59`	`59`
`60`		`- @Override`
`61`		`- public boolean blobExists(String blobName) {`
	`60`	`+ private boolean blobExists(String blobName) {`
`62`	`61`	`logger.trace("blobExists({})", blobName);`
`63`	`62`	`try {`
`64`	`63`	`return blobStore.blobExists(buildKey(blobName));`
Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,7 @@`
`31`	`31`	`import org.elasticsearch.common.blobstore.BlobStore;`
`32`	`32`	`import org.elasticsearch.common.bytes.BytesArray;`
`33`	`33`	`import org.elasticsearch.repositories.ESBlobStoreContainerTestCase;`
	`34`	`+import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;`
`34`	`35`
`35`	`36`	`import javax.security.auth.Subject;`
`36`	`37`	`import java.io.IOException;`
`@@ -137,6 +138,6 @@ public void testReadOnly() throws Exception {`
`137`	`138`	`byte[] data = randomBytes(randomIntBetween(10, scaledRandomIntBetween(1024, 1 << 16)));`
`138`	`139`	`writeBlob(container, "foo", new BytesArray(data), randomBoolean());`
`139`	`140`	`assertArrayEquals(readBlobFully(container, "foo", data.length), data);`
`140`		`- assertTrue(container.blobExists("foo"));`
	`141`	`+ assertTrue(BlobStoreTestUtil.blobExists(container, "foo"));`
`141`	`142`	`}`
`142`	`143`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1033,7 +1033,7 @@ public void verify(String seed, DiscoveryNode localNode) {`
`1033`	`1033`	`}`
`1034`	`1034`	`} else {`
`1035`	`1035`	`BlobContainer testBlobContainer = blobStore().blobContainer(basePath().add(testBlobPrefix(seed)));`
`1036`		`- if (testBlobContainer.blobExists("master.dat")) {`
	`1036`	`+ try (InputStream ignored = testBlobContainer.readBlob("master.dat")) {`
`1037`	`1037`	`try {`
`1038`	`1038`	`BytesArray bytes = new BytesArray(seed);`
`1039`	`1039`	`try (InputStream stream = bytes.streamInput()) {`
`@@ -1043,11 +1043,13 @@ public void verify(String seed, DiscoveryNode localNode) {`
`1043`	`1043`	`throw new RepositoryVerificationException(metadata.name(), "store location [" + blobStore() +`
`1044`	`1044`	`"] is not accessible on the node [" + localNode + "]", exp);`
`1045`	`1045`	`}`
`1046`		`- } else {`
	`1046`	`+ } catch (NoSuchFileException e) {`
`1047`	`1047`	`throw new RepositoryVerificationException(metadata.name(), "a file written by master to the store [" + blobStore() +`
`1048`	`1048`	`"] cannot be accessed on the node [" + localNode + "]. " +`
`1049`	`1049`	`"This might indicate that the store [" + blobStore() + "] is not shared between this node and the master node or " +`
`1050`		`- "that permissions on the store don't allow reading files written by the master node");`
	`1050`	`+ "that permissions on the store don't allow reading files written by the master node", e);`
	`1051`	`+ } catch (IOException e) {`
	`1052`	`+ throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e);`
`1051`	`1053`	`}`
`1052`	`1054`	`}`
`1053`	`1055`	`}`