Implement Bulk Deletes for GCS Repository by original-brownbear · Pull Request #41368 · elastic/elasticsearch

original-brownbear · 2019-04-19T10:22:22Z

Just like Add Bulk Delete Api to BlobStore #40322 for AWS
We already had a bulk delete API but weren't using it from the blob container implementation, now we are using it
- Made the bulk delete API also compliant with our interface that only suppresses errors about non-existent blobs by stating failed deletes (I didn't use any bulk stat action here since having to stat here should be the exception anyway and it would make error handling a lot more complex)
Fixed bulk delete API to limit its batch size to 100 in line with GCS recommendations

* Just like #40322 for AWS * We already had a bulk delete API but weren't using it from the blob container implementation, now we are using it * Made the bulk delete API also compliant with our interface that only suppresses errors about non existent blobs by stating failed deletes (I didn't use any bulk stat action here since having to stat here should be the exception anyway and it would make error handling a lot more complex) * Fixed bulk delete API to limit its batch size to 100 in line with GCS recommendations

elasticmachine · 2019-04-19T10:22:24Z

Pinging @elastic/es-distributed

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

ywelsch

AFAICS, Google's SDK already implements the max batch size (see HttpStorageRpc.DefaultRpcBatch) so no need to manually implement it.

original-brownbear · 2019-04-23T07:25:22Z

@ywelsch 🤦‍♂️ right ... obviously I only looked through their code to find the batch size and apparently stopped thinking at that point :)
=> batching removed in b971752 :)

andrershov

LGTM

original-brownbear · 2019-04-25T18:18:18Z

@ywelsch you're good with this one too? :)

ywelsch

AFAICS, what we would like to implement is the deleteBlobsIgnoringIfNotExists method. The behavior of List<Boolean> delete(Iterable<BlobId> blobIds) method in the Storage class of Google's SDK equate's failure and missing file:

@return an immutable list of booleans. If a blob has been deleted the corresponding item in the
   *     list is {@code true}. If a blob was not found, deletion failed or access to the resource
   *     was denied the corresponding item is {@code false}.

We want the distinction between the two, and not throw an exception if the file is just missing. This can be achieved by using the lower-level batch() method, building the batch yourself and then plugging in a different error handling mapping.

We should also have tests that check that this behavior is correctly implemented by any deleteBlobsIgnoringIfNotExists method.

On a related note, we should delete the "delete" method from the BlobStore interface, and replace it's single use by a blobcontainer.

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

original-brownbear · 2019-04-26T14:11:55Z

@ywelsch two 🤦‍♂️ in one PR from me :( Obviously much better to save the get calls and just use the proper callbacks.
Did that now in a029e7b :) (had to do some nasty mocking because the related classes had package private constructors in the GCloud SDK but other than that, this should work fine).

original-brownbear · 2019-04-26T14:28:46Z

Jenkins run elasticsearch-ci/1

original-brownbear · 2019-04-26T16:36:32Z

Jenkins run elasticsearch-ci/1

original-brownbear · 2019-04-27T05:43:31Z

@ywelsch

On a related note, we should delete the "delete" method from the BlobStore interface, and replace it's single use by a blobcontainer.

#41619 :)

ywelsch

We should also have tests that check that this behavior is correctly implemented by any deleteBlobsIgnoringIfNotExists method.

Can you also address that one? In particular, I'm thinking about ESBlobStoreContainerTestCase

ywelsch · 2019-04-29T07:26:05Z

...ry-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainer.java

-    protected String buildKey(String blobName) {
+    @Override
+    public void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOException {
+        blobStore.deleteBlobs(blobNames.stream().map(this::buildKey).collect(Collectors.toList()));


the method on blobstore should now also be renamed to deleteBlobsIgnoringIfNotExists

ywelsch · 2019-04-29T07:29:26Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

+
+                        @Override
+                        public void error(StorageException exception) {
+                            if (exception.getCode() == 404) {


let's use the constant here: HttpURLConnection.HTTP_NOT_FOUND

ywelsch · 2019-04-29T07:32:43Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

+                            if (exception.getCode() == 404) {
+                                results.add(Boolean.TRUE);
+                            } else {
+                                results.add(Boolean.FALSE);


I would prefer to have this consistent with the S3 implementation, which currently collects the exceptions and add them as suppressed.

ywelsch · 2019-04-29T07:33:12Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

        }
        if (failed) {
-            throw new IOException("Failed to delete all [" + blobIdsToDelete.size() + "] blobs");
+            throw new IOException("Failed to delete the following blobs " + blobIdsToDelete);


perhaps just "Failed to delete blobs " + blobIdsToDelete

ywelsch · 2019-04-29T07:35:00Z

plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/MockStorage.java

+            callback.success(true);
+            return null;
+        }).when(result).notify(any(BatchResult.Callback.class));
+        when(batch.delete(any(BlobId.class), anyVararg())).thenAnswer(invocation -> {


can we somehow check that no other method is called on this mock?

Jup done :)

original-brownbear · 2019-04-29T08:20:59Z

@ywelsch

Can you also address that one? In particular, I'm thinking about ESBlobStoreContainerTestCase

Oh sorry failed to answer here. We already have that test in place IMO: https://github.com/elastic/elasticsearch/blob/master/test/framework/src/main/java/org/elasticsearch/repositories/ESBlobStoreContainerTestCase.java#L140

The other points I addressed in
77e5875 and
d5a05fa

ywelsch · 2019-04-29T10:43:18Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

+            return ioe.get();
+        });
+        if (e != null) {
+            throw e;


this now throws the StorageException, and not an IOException like S3's BlobContainer. Let's use one pattern (the one we had for S3), not two different ones.

Sure, fixed in 1d8d9c0 :)

original-brownbear · 2019-04-29T11:22:22Z

Jenkins run elasticsearch-ci/packaging-sample

ywelsch · 2019-04-30T07:43:02Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

+            return ioe.get();
+        });
+        if (e != null) {
+            throw new IOException(e);


Also add a message just as for S3BlobContainer? i.e. "Exception when deleting blobs [" + blobNames + "]"

I consciously didn't do that here, since it's a little different from the AWS case in that we will always submit the full batch of deletes since the sub-batches are internal to the client (unlike in S3 where we do the splitting up of the batch).
So I expect us to always get an exception for every failed blob, making listing them again a little redundant (plus we're catching these exceptions upstream anyway and logging the blobs that we tried to delete)?
Maybe rather remove the listing from S3 as well? (I didn't realize it at the time, but it seems completely redundant when we always log the list of blobs upstream anyway)

It may be nicer to collect the files that failed deletion here than at the caller site. It allows filtering the list down to the actual file deletions that failed (i.e. instead of just blobNames, we can filter it down to those that actually experienced a failure). A similar thing can be done for S3.

Makes sense. Maybe keep it all in the exception though and simply make sure that the blob names are all in the exception?
I don't like manually collecting the list of failed deletes without exceptions tbh., it doesn't really give us any information. We want to know why a delete failed. The fact that it failed we can see by listing the stale blobs later on already :P

Then we could simply remove the blobs list from all the upstream logging and be done?
=> how about keeping this like they are here and adjusting the S3 implementation and upstream logging in a subsequent PR?

ok to adapt call sites in a follow-up PR, but let's add the message with all the blob names at least in this PR here.

original-brownbear · 2019-04-30T10:38:08Z

thanks @ywelsch, @andrershov !

* Implement Bulk Deletes for GCS Repository (#41368) * Just like #40322 for AWS * We already had a bulk delete API but weren't using it from the blob container implementation, now we are using it * Made the bulk delete API also compliant with our interface that only suppresses errors about non existent blobs by stating failed deletes (I didn't use any bulk stat action here since having to stat here should be the exception anyway and it would make error handling a lot more complex) * Fixed bulk delete API to limit its batch size to 100 in line with GCS recommendations back port of #41368

* Implement Bulk Deletes for GCS Repository * Just like elastic#40322 for AWS * We already had a bulk delete API but weren't using it from the blob container implementation, now we are using it * Made the bulk delete API also compliant with our interface that only suppresses errors about non existent blobs by stating failed deletes (I didn't use any bulk stat action here since having to stat here should be the exception anyway and it would make error handling a lot more complex) * Fixed bulk delete API to limit its batch size to 100 in line with GCS recommendations

* Cleanup Bulk Delete Exception Logging * Follow up to #41368 * Collect all failed blob deletes and add them to the exception message * Remove logging of blob name list from caller exception logging

* Implement Bulk Deletes for GCS Repository * Just like elastic#40322 for AWS * We already had a bulk delete API but weren't using it from the blob container implementation, now we are using it * Made the bulk delete API also compliant with our interface that only suppresses errors about non existent blobs by stating failed deletes (I didn't use any bulk stat action here since having to stat here should be the exception anyway and it would make error handling a lot more complex) * Fixed bulk delete API to limit its batch size to 100 in line with GCS recommendations

* Cleanup Bulk Delete Exception Logging * Follow up to elastic#41368 * Collect all failed blob deletes and add them to the exception message * Remove logging of blob name list from caller exception logging

* Cleanup Bulk Delete Exception Logging * Follow up to #41368 * Collect all failed blob deletes and add them to the exception message * Remove logging of blob name list from caller exception logging

original-brownbear added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.2.0 labels Apr 19, 2019

original-brownbear requested review from DaveCTurner and andrershov April 19, 2019 12:59

andrershov suggested changes Apr 19, 2019

View reviewed changes

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java Outdated Show resolved Hide resolved

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java Outdated Show resolved Hide resolved

original-brownbear requested review from andrershov and ywelsch April 19, 2019 15:46

ywelsch suggested changes Apr 23, 2019

View reviewed changes

original-brownbear added 2 commits April 23, 2019 09:18

CR: no need for manual batching

b971752

Merge remote-tracking branch 'elastic/master' into gcs-batch-delete

1ae7460

original-brownbear requested a review from ywelsch April 23, 2019 07:25

andrershov approved these changes Apr 25, 2019

View reviewed changes

ywelsch suggested changes Apr 26, 2019

View reviewed changes

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java Outdated Show resolved Hide resolved

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java Outdated Show resolved Hide resolved

original-brownbear added 2 commits April 26, 2019 15:23

Merge remote-tracking branch 'elastic/master' into gcs-batch-delete

ae58d9d

CR: use low level delete to not have to stat blobs

a029e7b

original-brownbear requested a review from ywelsch April 26, 2019 14:12

ywelsch suggested changes Apr 29, 2019

View reviewed changes

original-brownbear added 3 commits April 29, 2019 09:46

Merge remote-tracking branch 'elastic/master' into gcs-batch-delete

4199c2a

CR: rename method and better mock

77e5875

CR: collect exceptions

d5a05fa

original-brownbear requested a review from ywelsch April 29, 2019 08:21

ywelsch reviewed Apr 29, 2019

View reviewed changes

original-brownbear added 2 commits April 29, 2019 12:56

Merge remote-tracking branch 'elastic/master' into gcs-batch-delete

729e8b9

CR: throw IOException instead of StorageException

1d8d9c0

original-brownbear requested a review from ywelsch April 29, 2019 10:58

ywelsch reviewed Apr 30, 2019

View reviewed changes

Merge remote-tracking branch 'elastic/master' into gcs-batch-delete

2838d3d

original-brownbear requested a review from ywelsch April 30, 2019 08:16

ywelsch approved these changes Apr 30, 2019

View reviewed changes

CR: collect list of failed blob names in ex message

a0e616c

original-brownbear merged commit b633182 into elastic:master Apr 30, 2019

original-brownbear deleted the gcs-batch-delete branch April 30, 2019 10:38

original-brownbear added backport pending and removed backport pending labels Apr 30, 2019

original-brownbear mentioned this pull request Apr 30, 2019

Implement Bulk Deletes for GCS Repository (#41368) #41681

Merged

original-brownbear mentioned this pull request Apr 30, 2019

Cleanup Bulk Delete Exception Logging #41693

Merged

original-brownbear mentioned this pull request May 8, 2019

Remove Delete Method from BlobStore #41619

Merged

original-brownbear mentioned this pull request May 28, 2019

Cleanup Bulk Delete Exception Logging (#41693) #42606

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

original-brownbear commented Apr 19, 2019

Uh oh!

elasticmachine commented Apr 19, 2019

Uh oh!

Uh oh!

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 23, 2019

Uh oh!

andrershov left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 25, 2019

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

original-brownbear commented Apr 26, 2019

Uh oh!

original-brownbear commented Apr 26, 2019

Uh oh!

original-brownbear commented Apr 26, 2019

Uh oh!

original-brownbear commented Apr 27, 2019

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

original-brownbear commented Apr 30, 2019 •

edited

Loading