diff --git a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleDataSegmentKiller.java b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleDataSegmentKiller.java index c0dc4c8ea33..42cc7c194b4 100644 --- a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleDataSegmentKiller.java +++ b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleDataSegmentKiller.java @@ -19,13 +19,12 @@ package org.apache.druid.storage.google; -import com.google.api.client.http.HttpResponseException; +import com.google.cloud.storage.StorageException; import com.google.common.base.Predicates; import com.google.inject.Inject; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.MapUtils; import org.apache.druid.java.util.common.RE; -import org.apache.druid.java.util.common.RetryUtils; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.segment.loading.DataSegmentKiller; import org.apache.druid.segment.loading.SegmentLoadingException; @@ -70,35 +69,24 @@ public class GoogleDataSegmentKiller implements DataSegmentKiller // anymore, but we still delete them if exists. deleteIfPresent(bucket, descriptorPath); } - catch (IOException e) { + catch (StorageException e) { throw new SegmentLoadingException(e, "Couldn't kill segment[%s]: [%s]", segment.getId(), e.getMessage()); } } - private void deleteIfPresent(String bucket, String path) throws IOException + private void deleteIfPresent(String bucket, String path) { try { - RetryUtils.retry( - (RetryUtils.Task) () -> { - storage.delete(bucket, path); - return null; - }, - GoogleUtils::isRetryable, - 1, - 5 - ); + GoogleUtils.retryGoogleCloudStorageOperation(() -> { + storage.delete(bucket, path); + return null; + }); } - catch (HttpResponseException e) { - if (e.getStatusCode() != 404) { - throw e; - } - LOG.debug("Already deleted: [%s] [%s]", bucket, path); - } - catch (IOException ioe) { - throw ioe; + catch (StorageException e) { + throw e; } catch (Exception e) { - throw new RE(e, "Failed to delete [%s] [%s]", bucket, path); + throw new RE(e, "Failed to delete google cloud storage object from bucket [%s] and path [%s].", bucket, path); } } diff --git a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java index 91d290b1785..15cca5e08d5 100644 --- a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java +++ b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorage.java @@ -31,6 +31,7 @@ import com.google.common.base.Supplier; import com.google.common.collect.Iterables; import org.apache.druid.java.util.common.HumanReadableBytes; import org.apache.druid.java.util.common.IOE; +import org.apache.druid.java.util.common.logger.Logger; import javax.annotation.Nullable; import java.io.IOException; @@ -52,6 +53,8 @@ public class GoogleStorage *

* See OmniDataSegmentKiller for how DataSegmentKillers are initialized. */ + private static final Logger log = new Logger(GoogleStorage.class); + private final Supplier storage; private final HumanReadableBytes DEFAULT_WRITE_CHUNK_SIZE = new HumanReadableBytes("4MiB"); @@ -131,7 +134,7 @@ public class GoogleStorage { Blob blob = storage.get().get(bucket, path, Storage.BlobGetOption.fields(Storage.BlobField.values())); if (blob == null) { - throw new IOE("Failed fetching google cloud storage object [bucket: %s, path: %s]", bucket, path); + throw new IOE("Failed to fetch google cloud storage object from bucket [%s] and path[%s].", bucket, path); } return new GoogleStorageObjectMetadata( blob.getBucket(), @@ -142,28 +145,41 @@ public class GoogleStorage ); } - public void delete(final String bucket, final String path) throws IOException + + /** + * Deletes an object in a bucket on the specified path + + * A false response from GCS delete API is indicative of file not found. Any other error is raised as a StorageException + * and should be explicitly handled. + Ref: HttpStorageRpc.java + * + * @param bucket GCS bucket + * @param path Object path + */ + public void delete(final String bucket, final String path) { if (!storage.get().delete(bucket, path)) { - throw new IOE( - "Failed deleting google cloud storage object [bucket: %s path: %s]", - bucket, - path - ); + log.debug("Google cloud storage object to be deleted not found in bucket [%s] and path [%s].", bucket, path); } } /** * Deletes a list of objects in a bucket + * A false response from GCS delete API is indicative of file not found. Any other error is raised as a StorageException + * and should be explicitly handled. + * Ref: HttpStorageRpc.java * * @param bucket GCS bucket * @param paths Iterable for absolute paths of objects to be deleted inside the bucket */ - public void batchDelete(final String bucket, final Iterable paths) throws IOException + public void batchDelete(final String bucket, final Iterable paths) { - List statuses = storage.get().delete(Iterables.transform(paths, input -> BlobId.of(bucket, input))); + final List statuses = storage.get().delete(Iterables.transform(paths, input -> BlobId.of(bucket, input))); if (statuses.contains(false)) { - throw new IOE("Failed deleting google cloud storage object(s)"); + log.debug( + "Google cloud storage object(s) to be deleted not found in bucket [%s].", + bucket + ); } } @@ -177,7 +193,7 @@ public class GoogleStorage { Blob blob = storage.get().get(bucket, path, Storage.BlobGetOption.fields(Storage.BlobField.SIZE)); if (blob == null) { - throw new IOE("Failed fetching google cloud storage object [bucket: %s, path: %s]", bucket, path); + throw new IOE("Failed to fetch google cloud storage object from bucket [%s] and path [%s].", bucket, path); } return blob.getSize(); } @@ -186,7 +202,7 @@ public class GoogleStorage { Blob blob = storage.get().get(bucket, path, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION)); if (blob == null) { - throw new IOE("Failed fetching google cloud storage object [bucket: %s, path: %s]", bucket, path); + throw new IOE("Failed to fetch google cloud storage object from bucket [%s] and path [%s].", bucket, path); } return blob.getGeneratedId(); } @@ -223,7 +239,7 @@ public class GoogleStorage Page blobPage = storage.get().list(bucket, options.toArray(new Storage.BlobListOption[0])); if (blobPage == null) { - throw new IOE("Failed fetching google cloud storage object [bucket: %s, prefix: %s]", bucket, prefix); + throw new IOE("Failed to fetch google cloud storage object from bucket [%s] and prefix [%s].", bucket, prefix); } @@ -247,6 +263,5 @@ public class GoogleStorage { BlobId blobId = BlobId.of(bucket, path); return BlobInfo.newBuilder(blobId).build(); - } } diff --git a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleUtils.java b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleUtils.java index a819442ef35..cba718ac338 100644 --- a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleUtils.java +++ b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleUtils.java @@ -20,6 +20,7 @@ package org.apache.druid.storage.google; import com.google.api.client.http.HttpResponseException; +import com.google.cloud.storage.StorageException; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import org.apache.druid.data.input.impl.CloudObjectLocation; @@ -40,6 +41,9 @@ public class GoogleUtils if (t instanceof HttpResponseException) { final HttpResponseException e = (HttpResponseException) t; return e.getStatusCode() == 429 || (e.getStatusCode() / 500 == 1); + } else if (t instanceof StorageException) { + final StorageException e = (StorageException) t; + return e.isRetryable(); } return t instanceof IOException; } diff --git a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/output/GoogleStorageConnector.java b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/output/GoogleStorageConnector.java index 6edbad3beaf..0d147cce25a 100644 --- a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/output/GoogleStorageConnector.java +++ b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/output/GoogleStorageConnector.java @@ -73,7 +73,7 @@ public class GoogleStorageConnector extends ChunkingStorageConnector { @@ -105,7 +105,7 @@ public class GoogleStorageConnector extends ChunkingStorageConnector paths) throws IOException { - storage.batchDelete(config.getBucket(), Iterables.transform(paths, this::objectPath)); + try { + GoogleUtils.retryGoogleCloudStorageOperation(() -> { + storage.batchDelete(config.getBucket(), Iterables.transform(paths, this::objectPath)); + return null; + }); + } + catch (Exception e) { + log.error("Failed to delete object(s) at bucket [%s].", config.getBucket()); + throw new IOException(e); + } + } @Override @@ -127,10 +137,19 @@ public class GoogleStorageConnector extends ChunkingStorageConnector Iterators.transform(storageObjects, GoogleStorageObjectMetadata::getName) - ); + try { + GoogleUtils.retryGoogleCloudStorageOperation(() -> { + storage.batchDelete( + config.getBucket(), + () -> Iterators.transform(storageObjects, GoogleStorageObjectMetadata::getName) + ); + return null; + }); + } + catch (Exception e) { + log.error("Failed to delete object(s) at bucket [%s] and prefix [%s].", config.getBucket(), fullPath); + throw new IOException(e); + } } @Override diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleDataSegmentKillerTest.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleDataSegmentKillerTest.java index 8d9612f4d8d..87b406b1fa4 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleDataSegmentKillerTest.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleDataSegmentKillerTest.java @@ -19,11 +19,7 @@ package org.apache.druid.storage.google; -import com.google.api.client.googleapis.json.GoogleJsonResponseException; -import com.google.api.client.googleapis.testing.json.GoogleJsonResponseExceptionFactoryTesting; -import com.google.api.client.http.HttpHeaders; -import com.google.api.client.http.HttpResponseException; -import com.google.api.client.json.jackson2.JacksonFactory; +import com.google.cloud.storage.StorageException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.apache.druid.java.util.common.ISE; @@ -54,8 +50,8 @@ public class GoogleDataSegmentKillerTest extends EasyMockSupport private static final long TIME_0 = 0L; private static final long TIME_1 = 1L; private static final int MAX_KEYS = 1; - private static final Exception RECOVERABLE_EXCEPTION = new HttpResponseException.Builder(429, "recoverable", new HttpHeaders()).build(); - private static final Exception NON_RECOVERABLE_EXCEPTION = new HttpResponseException.Builder(404, "non recoverable", new HttpHeaders()).build(); + private static final Exception RECOVERABLE_EXCEPTION = new StorageException(429, "recoverable"); + private static final Exception NON_RECOVERABLE_EXCEPTION = new StorageException(404, "non-recoverable"); private static final DataSegment DATA_SEGMENT = new DataSegment( @@ -83,7 +79,7 @@ public class GoogleDataSegmentKillerTest extends EasyMockSupport } @Test - public void killTest() throws SegmentLoadingException, IOException + public void killTest() throws SegmentLoadingException { storage.delete(EasyMock.eq(BUCKET), EasyMock.eq(INDEX_PATH)); EasyMock.expectLastCall(); @@ -99,38 +95,30 @@ public class GoogleDataSegmentKillerTest extends EasyMockSupport verifyAll(); } - @Test(expected = SegmentLoadingException.class) - public void killWithErrorTest() throws SegmentLoadingException, IOException + @Test + public void killWithErrorTest() { - final GoogleJsonResponseException exception = GoogleJsonResponseExceptionFactoryTesting.newMock( - JacksonFactory.getDefaultInstance(), - 300, - "test" - ); - storage.delete(EasyMock.eq(BUCKET), EasyMock.eq(INDEX_PATH)); - EasyMock.expectLastCall().andThrow(exception); + Assert.assertThrows(SegmentLoadingException.class, () -> { + storage.delete(EasyMock.eq(BUCKET), EasyMock.eq(INDEX_PATH)); + EasyMock.expectLastCall().andThrow(NON_RECOVERABLE_EXCEPTION); - replayAll(); + replayAll(); - GoogleDataSegmentKiller killer = new GoogleDataSegmentKiller(storage, accountConfig, inputDataConfig); + GoogleDataSegmentKiller killer = new GoogleDataSegmentKiller(storage, accountConfig, inputDataConfig); - killer.kill(DATA_SEGMENT); + killer.kill(DATA_SEGMENT); - verifyAll(); + verifyAll(); + }); } @Test - public void killRetryWithErrorTest() throws SegmentLoadingException, IOException + public void killRetryWithErrorTest() throws SegmentLoadingException { - final GoogleJsonResponseException exception = GoogleJsonResponseExceptionFactoryTesting.newMock( - JacksonFactory.getDefaultInstance(), - 500, - "test" - ); storage.delete(EasyMock.eq(BUCKET), EasyMock.eq(INDEX_PATH)); - EasyMock.expectLastCall().andThrow(exception).once().andVoid().once(); + EasyMock.expectLastCall().andThrow(RECOVERABLE_EXCEPTION).once().andVoid().once(); storage.delete(EasyMock.eq(BUCKET), EasyMock.eq(DESCRIPTOR_PATH)); - EasyMock.expectLastCall().andThrow(exception).once().andVoid().once(); + EasyMock.expectLastCall().andThrow(RECOVERABLE_EXCEPTION).once().andVoid().once(); replayAll(); @@ -142,25 +130,16 @@ public class GoogleDataSegmentKillerTest extends EasyMockSupport } @Test - public void test_killAll_accountConfigWithNullBucketAndPrefix_throwsISEException() throws IOException + public void test_killAll_accountConfigWithNullBucketAndPrefix_throwsISEException() { EasyMock.expect(accountConfig.getBucket()).andReturn(null).atLeastOnce(); EasyMock.expect(accountConfig.getPrefix()).andReturn(null).anyTimes(); + GoogleDataSegmentKiller killer = new GoogleDataSegmentKiller(storage, accountConfig, inputDataConfig); + EasyMock.replay(storage, inputDataConfig, accountConfig); - boolean thrownISEException = false; + Assert.assertThrows(ISE.class, killer::killAll); - try { - GoogleDataSegmentKiller killer = new GoogleDataSegmentKiller(storage, accountConfig, inputDataConfig); - EasyMock.replay(storage, inputDataConfig, accountConfig); - - killer.killAll(); - } - catch (ISE e) { - thrownISEException = true; - } - - Assert.assertTrue(thrownISEException); EasyMock.verify(accountConfig, inputDataConfig, storage); } @@ -217,34 +196,27 @@ public class GoogleDataSegmentKillerTest extends EasyMockSupport } @Test - public void test_killAll_nonrecoverableExceptionWhenListingObjects_doesntDeleteAnyTaskLogs() + public void test_killAll_nonrecoverableExceptionWhenListingObjects_doesntDeleteAnyTaskLogs() throws IOException { - boolean ioExceptionThrown = false; - try { - GoogleStorageObjectMetadata object1 = GoogleTestUtils.newStorageObject(BUCKET, KEY_1, TIME_0); + GoogleStorageObjectMetadata object1 = GoogleTestUtils.newStorageObject(BUCKET, KEY_1, TIME_0); - GoogleTestUtils.expectListObjectsPageRequest(storage, PREFIX_URI, MAX_KEYS, ImmutableList.of(object1)); + GoogleTestUtils.expectListObjectsPageRequest(storage, PREFIX_URI, MAX_KEYS, ImmutableList.of(object1)); - GoogleTestUtils.expectDeleteObjects( - storage, - ImmutableList.of(), - ImmutableMap.of(object1, NON_RECOVERABLE_EXCEPTION) - ); + GoogleTestUtils.expectDeleteObjects( + storage, + ImmutableList.of(), + ImmutableMap.of(object1, NON_RECOVERABLE_EXCEPTION) + ); - EasyMock.expect(accountConfig.getBucket()).andReturn(BUCKET).anyTimes(); - EasyMock.expect(accountConfig.getPrefix()).andReturn(PREFIX).anyTimes(); - EasyMock.expect(inputDataConfig.getMaxListingLength()).andReturn(MAX_KEYS); + EasyMock.expect(accountConfig.getBucket()).andReturn(BUCKET).anyTimes(); + EasyMock.expect(accountConfig.getPrefix()).andReturn(PREFIX).anyTimes(); + EasyMock.expect(inputDataConfig.getMaxListingLength()).andReturn(MAX_KEYS); - EasyMock.replay(accountConfig, inputDataConfig, storage); + EasyMock.replay(accountConfig, inputDataConfig, storage); - GoogleDataSegmentKiller killer = new GoogleDataSegmentKiller(storage, accountConfig, inputDataConfig); - killer.killAll(); - } - catch (IOException e) { - ioExceptionThrown = true; - } + GoogleDataSegmentKiller killer = new GoogleDataSegmentKiller(storage, accountConfig, inputDataConfig); - Assert.assertTrue(ioExceptionThrown); + Assert.assertThrows(IOException.class, killer::killAll); EasyMock.verify(accountConfig, inputDataConfig, storage); } diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageTest.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageTest.java index d92339f53c7..f227d593ce6 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageTest.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageTest.java @@ -23,9 +23,11 @@ import com.google.api.gax.paging.Page; import com.google.cloud.storage.Blob; import com.google.cloud.storage.BlobId; import com.google.cloud.storage.Storage; +import com.google.cloud.storage.StorageException; import com.google.common.collect.ImmutableList; import org.easymock.Capture; import org.easymock.EasyMock; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -50,6 +52,8 @@ public class GoogleStorageTest static final String PATH = "/path"; static final long SIZE = 100; static final OffsetDateTime UPDATE_TIME = OffsetDateTime.MIN; + private static final Exception STORAGE_EXCEPTION = new StorageException(404, "Runtime Storage Exception"); + @Before public void setUp() @@ -62,7 +66,7 @@ public class GoogleStorageTest } @Test - public void testDeleteSuccess() throws IOException + public void testDeleteSuccess() { EasyMock.expect(mockStorage.delete(EasyMock.eq(BUCKET), EasyMock.eq(PATH))).andReturn(true); EasyMock.replay(mockStorage); @@ -70,23 +74,23 @@ public class GoogleStorageTest } @Test - public void testDeleteFailure() + public void testDeleteFileNotFound() { EasyMock.expect(mockStorage.delete(EasyMock.eq(BUCKET), EasyMock.eq(PATH))).andReturn(false); EasyMock.replay(mockStorage); - boolean thrownIOException = false; - try { - googleStorage.delete(BUCKET, PATH); - - } - catch (IOException e) { - thrownIOException = true; - } - assertTrue(thrownIOException); + googleStorage.delete(BUCKET, PATH); } @Test - public void testBatchDeleteSuccess() throws IOException + public void testDeleteFailure() + { + EasyMock.expect(mockStorage.delete(EasyMock.eq(BUCKET), EasyMock.eq(PATH))).andThrow(STORAGE_EXCEPTION); + EasyMock.replay(mockStorage); + Assert.assertThrows(StorageException.class, () -> googleStorage.delete(BUCKET, PATH)); + } + + @Test + public void testBatchDeleteSuccess() { List paths = ImmutableList.of("/path1", "/path2"); final Capture> pathIterable = Capture.newInstance(); @@ -103,6 +107,29 @@ public class GoogleStorageTest assertTrue(paths.size() == recordedPaths.size() && paths.containsAll(recordedPaths) && recordedPaths.containsAll( paths)); assertEquals(BUCKET, recordedBlobIds.get(0).getBucket()); + + } + + @Test + public void testBatchDeleteFileNotFound() + { + List paths = ImmutableList.of("/path1", "/path2"); + final Capture> pathIterable = Capture.newInstance(); + EasyMock.expect(mockStorage.delete(EasyMock.capture(pathIterable))).andReturn(ImmutableList.of(true, false)); + EasyMock.replay(mockStorage); + + googleStorage.batchDelete(BUCKET, paths); + + List recordedBlobIds = new ArrayList<>(); + pathIterable.getValue().iterator().forEachRemaining(recordedBlobIds::add); + + List recordedPaths = recordedBlobIds.stream().map(BlobId::getName).collect(Collectors.toList()); + + assertTrue(paths.size() == recordedPaths.size()); + assertTrue(paths.containsAll(recordedPaths)); + assertTrue(recordedPaths.containsAll(paths)); + assertEquals(BUCKET, recordedBlobIds.get(0).getBucket()); + } @Test @@ -110,17 +137,9 @@ public class GoogleStorageTest { List paths = ImmutableList.of("/path1", "/path2"); EasyMock.expect(mockStorage.delete((Iterable) EasyMock.anyObject())) - .andReturn(ImmutableList.of(false, true)); + .andThrow(STORAGE_EXCEPTION); EasyMock.replay(mockStorage); - boolean thrownIOException = false; - try { - googleStorage.batchDelete(BUCKET, paths); - - } - catch (IOException e) { - thrownIOException = true; - } - assertTrue(thrownIOException); + Assert.assertThrows(StorageException.class, () -> googleStorage.batchDelete(BUCKET, paths)); } @Test diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTaskLogsTest.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTaskLogsTest.java index a0f17c97d91..438d4b8ed6f 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTaskLogsTest.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTaskLogsTest.java @@ -19,9 +19,8 @@ package org.apache.druid.storage.google; -import com.google.api.client.http.HttpHeaders; -import com.google.api.client.http.HttpResponseException; import com.google.api.client.http.InputStreamContent; +import com.google.cloud.storage.StorageException; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -58,8 +57,8 @@ public class GoogleTaskLogsTest extends EasyMockSupport private static final long TIME_NOW = 2L; private static final long TIME_FUTURE = 3L; private static final int MAX_KEYS = 1; - private static final Exception RECOVERABLE_EXCEPTION = new HttpResponseException.Builder(429, "recoverable", new HttpHeaders()).build(); - private static final Exception NON_RECOVERABLE_EXCEPTION = new HttpResponseException.Builder(404, "non recoverable", new HttpHeaders()).build(); + private static final Exception RECOVERABLE_EXCEPTION = new StorageException(429, "recoverable"); + private static final Exception NON_RECOVERABLE_EXCEPTION = new StorageException(404, "non-recoverable"); private GoogleStorage storage; private GoogleTaskLogs googleTaskLogs; diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTestUtils.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTestUtils.java index c68911448e2..032ab08a066 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTestUtils.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleTestUtils.java @@ -71,7 +71,7 @@ public class GoogleTestUtils extends EasyMockSupport GoogleStorage storage, List deleteObjectExpected, Map deleteObjectToException - ) throws IOException + ) { Map> requestToResultExpectationSetter = new HashMap<>(); for (Map.Entry deleteObjectAndException : deleteObjectToException.entrySet()) { diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleUtilsTest.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleUtilsTest.java index aac7ab0fbf7..76f6dfe9817 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleUtilsTest.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleUtilsTest.java @@ -22,6 +22,7 @@ package org.apache.druid.storage.google; import com.google.api.client.googleapis.json.GoogleJsonResponseException; import com.google.api.client.http.HttpHeaders; import com.google.api.client.http.HttpResponseException; +import com.google.cloud.storage.StorageException; import org.junit.Assert; import org.junit.Test; @@ -78,5 +79,30 @@ public class GoogleUtilsTest new IOException("generic io exception") ) ); + Assert.assertFalse( + GoogleUtils.isRetryable( + new StorageException(404, "ignored") + ) + ); + Assert.assertTrue( + GoogleUtils.isRetryable( + new StorageException(429, "ignored") + ) + ); + Assert.assertTrue( + GoogleUtils.isRetryable( + new StorageException(500, "ignored") + ) + ); + Assert.assertTrue( + GoogleUtils.isRetryable( + new StorageException(503, "ignored") + ) + ); + Assert.assertFalse( + GoogleUtils.isRetryable( + new StorageException(599, "ignored") + ) + ); } } diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/output/GoogleStorageConnectorTest.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/output/GoogleStorageConnectorTest.java index 7a5e6ba107b..c1cf4304c4b 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/output/GoogleStorageConnectorTest.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/output/GoogleStorageConnectorTest.java @@ -19,6 +19,7 @@ package org.apache.druid.storage.google.output; +import com.google.cloud.storage.StorageException; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import org.apache.commons.io.IOUtils; @@ -54,6 +55,8 @@ public class GoogleStorageConnectorTest GoogleStorageConnector googleStorageConnector; private final GoogleStorage googleStorage = EasyMock.createMock(GoogleStorage.class); + private static final Exception RECOVERABLE_EXCEPTION = new StorageException(429, "recoverable"); + private static final Exception NON_RECOVERABLE_EXCEPTION = new StorageException(404, "non-recoverable"); @Before public void setUp() throws IOException @@ -91,7 +94,7 @@ public class GoogleStorageConnectorTest } @Test - public void testDeleteFile() throws IOException + public void testDeleteFileSuccess() throws IOException { Capture bucketCapture = EasyMock.newCapture(); Capture pathCapture = EasyMock.newCapture(); @@ -107,7 +110,40 @@ public class GoogleStorageConnectorTest } @Test - public void testDeleteFiles() throws IOException + public void testDeleteFileRetrySuccess() throws IOException + { + Capture bucketCapture = EasyMock.newCapture(); + Capture pathCapture = EasyMock.newCapture(); + googleStorage.delete( + EasyMock.capture(bucketCapture), + EasyMock.capture(pathCapture) + ); + EasyMock.expectLastCall().andThrow(RECOVERABLE_EXCEPTION).once().andVoid().once(); + + EasyMock.replay(googleStorage); + googleStorageConnector.deleteFile(TEST_FILE); + Assert.assertEquals(BUCKET, bucketCapture.getValue()); + Assert.assertEquals(PREFIX + "/" + TEST_FILE, pathCapture.getValue()); + } + + @Test + public void testDeleteFileFailure() + { + Capture bucketCapture = EasyMock.newCapture(); + Capture pathCapture = EasyMock.newCapture(); + googleStorage.delete( + EasyMock.capture(bucketCapture), + EasyMock.capture(pathCapture) + ); + EasyMock.expectLastCall().andThrow(NON_RECOVERABLE_EXCEPTION).once().andVoid().once(); + EasyMock.replay(googleStorage); + Assert.assertThrows(IOException.class, () -> googleStorageConnector.deleteFile(TEST_FILE)); + Assert.assertEquals(BUCKET, bucketCapture.getValue()); + Assert.assertEquals(PREFIX + "/" + TEST_FILE, pathCapture.getValue()); + } + + @Test + public void testDeleteFilesSuccess() throws IOException { Capture containerCapture = EasyMock.newCapture(); Capture> pathsCapture = EasyMock.newCapture(); @@ -125,6 +161,191 @@ public class GoogleStorageConnectorTest EasyMock.reset(googleStorage); } + @Test + public void testDeleteFilesRetrySuccess() throws IOException + { + Capture containerCapture = EasyMock.newCapture(); + Capture> pathsCapture = EasyMock.newCapture(); + googleStorage.batchDelete(EasyMock.capture(containerCapture), EasyMock.capture(pathsCapture)); + EasyMock.expectLastCall().andThrow(RECOVERABLE_EXCEPTION).once().andVoid().once(); + + EasyMock.replay(googleStorage); + googleStorageConnector.deleteFiles(ImmutableList.of(TEST_FILE + "_1.part", TEST_FILE + "_2.json")); + Assert.assertEquals(BUCKET, containerCapture.getValue()); + Assert.assertEquals( + ImmutableList.of( + PREFIX + "/" + TEST_FILE + "_1.part", + PREFIX + "/" + TEST_FILE + "_2.json" + ), + Lists.newArrayList(pathsCapture.getValue()) + ); + EasyMock.reset(googleStorage); + } + + @Test + public void testDeleteFilesFailure() + { + Capture containerCapture = EasyMock.newCapture(); + Capture> pathsCapture = EasyMock.newCapture(); + googleStorage.batchDelete(EasyMock.capture(containerCapture), EasyMock.capture(pathsCapture)); + EasyMock.expectLastCall().andThrow(NON_RECOVERABLE_EXCEPTION).once().andVoid().once(); + + EasyMock.replay(googleStorage); + Assert.assertThrows( + IOException.class, + () -> googleStorageConnector.deleteFiles(ImmutableList.of( + TEST_FILE + "_1.part", + TEST_FILE + "_2.json" + )) + ); + Assert.assertEquals(BUCKET, containerCapture.getValue()); + Assert.assertEquals( + ImmutableList.of( + PREFIX + "/" + TEST_FILE + "_1.part", + PREFIX + "/" + TEST_FILE + "_2.json" + ), + Lists.newArrayList(pathsCapture.getValue()) + ); + EasyMock.reset(googleStorage); + } + + @Test + public void testDeleteFilesRecursivelySuccess() throws IOException + { + + GoogleStorageObjectMetadata objectMetadata1 = new GoogleStorageObjectMetadata( + BUCKET, + PREFIX + "/x/y/" + TEST_FILE, + (long) 3, + null + ); + + GoogleStorageObjectMetadata objectMetadata2 = new GoogleStorageObjectMetadata( + BUCKET, + PREFIX + "/p/q/r/" + TEST_FILE, + (long) 4, + null + ); + + Capture maxListingCapture = EasyMock.newCapture(); + Capture pageTokenCapture = EasyMock.newCapture(); + EasyMock.expect(googleStorage.list( + EasyMock.anyString(), + EasyMock.anyString(), + EasyMock.capture(maxListingCapture), + EasyMock.capture(pageTokenCapture) + )) + .andReturn(new GoogleStorageObjectPage(ImmutableList.of(objectMetadata1, objectMetadata2), null)); + + + Capture containerCapture = EasyMock.newCapture(); + Capture> pathsCapture = EasyMock.newCapture(); + googleStorage.batchDelete(EasyMock.capture(containerCapture), EasyMock.capture(pathsCapture)); + EasyMock.expectLastCall().andVoid().once(); + + EasyMock.replay(googleStorage); + googleStorageConnector.deleteRecursively(""); + Assert.assertEquals(BUCKET, containerCapture.getValue()); + Assert.assertEquals( + ImmutableList.of( + PREFIX + "/x/y/" + TEST_FILE, + PREFIX + "/p/q/r/" + TEST_FILE + ), + Lists.newArrayList(pathsCapture.getValue()) + ); + EasyMock.reset(googleStorage); + } + @Test + public void testDeleteFilesRecursivelyRetrySuccess() throws IOException + { + + GoogleStorageObjectMetadata objectMetadata1 = new GoogleStorageObjectMetadata( + BUCKET, + PREFIX + "/x/y/" + TEST_FILE, + (long) 3, + null + ); + + GoogleStorageObjectMetadata objectMetadata2 = new GoogleStorageObjectMetadata( + BUCKET, + PREFIX + "/p/q/r/" + TEST_FILE, + (long) 4, + null + ); + + Capture maxListingCapture = EasyMock.newCapture(); + Capture pageTokenCapture = EasyMock.newCapture(); + EasyMock.expect(googleStorage.list( + EasyMock.anyString(), + EasyMock.anyString(), + EasyMock.capture(maxListingCapture), + EasyMock.capture(pageTokenCapture) + )) + .andReturn(new GoogleStorageObjectPage(ImmutableList.of(objectMetadata1, objectMetadata2), null)); + + + Capture containerCapture = EasyMock.newCapture(); + Capture> pathsCapture = EasyMock.newCapture(); + googleStorage.batchDelete(EasyMock.capture(containerCapture), EasyMock.capture(pathsCapture)); + EasyMock.expectLastCall().andThrow(RECOVERABLE_EXCEPTION).once().andVoid().once(); + + EasyMock.replay(googleStorage); + googleStorageConnector.deleteRecursively(""); + Assert.assertEquals(BUCKET, containerCapture.getValue()); + Assert.assertEquals( + ImmutableList.of( + PREFIX + "/x/y/" + TEST_FILE, + PREFIX + "/p/q/r/" + TEST_FILE + ), + Lists.newArrayList(pathsCapture.getValue()) + ); + } + @Test + public void testDeleteFilesRecursivelyFailure() throws IOException + { + + GoogleStorageObjectMetadata objectMetadata1 = new GoogleStorageObjectMetadata( + BUCKET, + PREFIX + "/x/y/" + TEST_FILE, + (long) 3, + null + ); + + GoogleStorageObjectMetadata objectMetadata2 = new GoogleStorageObjectMetadata( + BUCKET, + PREFIX + "/p/q/r/" + TEST_FILE, + (long) 4, + null + ); + + Capture maxListingCapture = EasyMock.newCapture(); + Capture pageTokenCapture = EasyMock.newCapture(); + EasyMock.expect(googleStorage.list( + EasyMock.anyString(), + EasyMock.anyString(), + EasyMock.capture(maxListingCapture), + EasyMock.capture(pageTokenCapture) + )) + .andReturn(new GoogleStorageObjectPage(ImmutableList.of(objectMetadata1, objectMetadata2), null)); + + + Capture containerCapture = EasyMock.newCapture(); + Capture> pathsCapture = EasyMock.newCapture(); + googleStorage.batchDelete(EasyMock.capture(containerCapture), EasyMock.capture(pathsCapture)); + EasyMock.expectLastCall().andThrow(NON_RECOVERABLE_EXCEPTION).once().andVoid().once(); + + EasyMock.replay(googleStorage); + Assert.assertThrows(IOException.class, () -> googleStorageConnector.deleteRecursively("")); + Assert.assertEquals(BUCKET, containerCapture.getValue()); + Assert.assertEquals( + ImmutableList.of( + PREFIX + "/x/y/" + TEST_FILE, + PREFIX + "/p/q/r/" + TEST_FILE + ), + Lists.newArrayList(pathsCapture.getValue()) + ); + } + @Test public void testListDir() throws IOException { diff --git a/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/utils/GcsTestUtil.java b/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/utils/GcsTestUtil.java index 9c44d5b1439..ec67269e46d 100644 --- a/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/utils/GcsTestUtil.java +++ b/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/utils/GcsTestUtil.java @@ -99,7 +99,7 @@ public class GcsTestUtil ); } - public void deleteFileFromGcs(String gcsObjectName) throws IOException + public void deleteFileFromGcs(String gcsObjectName) { LOG.info("Deleting object %s at path %s in bucket %s", gcsObjectName, GOOGLE_PREFIX, GOOGLE_BUCKET); googleStorageClient.delete(GOOGLE_BUCKET, GOOGLE_PREFIX + "/" + gcsObjectName);