Add Bulk Delete Api to BlobStore by original-brownbear · Pull Request #40322 · elastic/elasticsearch

original-brownbear · 2019-03-21T16:09:21Z

Adds Bulk delete API to blob container
Implement bulk delete API for S3
- I'd look into other implementations in a subsequent PR if the approach here is ok. This would also be easier once Async Snapshot Repository Deletes #40144 is merged as it would probably be best to push the async logic down to the blob container to get more fine grained control (since we will want different parallelism depending on whether or not bulk deletes are available in an implementation I think)
Use bulk API instead of loops over single deletes
Adjust S3Fixture to accept both path styles for bulk deletes since the S3 SDK uses both during our ITs
Closes Add Bulk Delete API to Blob Store Interface #40250

For S3 this should pretty much give us almost 1000x speedup for deletes of huge indices/shards.

elasticmachine · 2019-03-21T16:09:23Z

Pinging @elastic/es-distributed

original-brownbear · 2019-03-21T16:52:10Z

Jenkins run elasticsearch-ci/bwc

andrershov

Thanks for the PR!
I've added a few comments. Also, I wonder if we need to add some kind of tests for deleteBlobs method?

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

andrershov · 2019-03-29T12:18:44Z

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

Will this implementation work for both cases? It seems that {bucket} is even not used in the implementation

Yea, the unfortunate reality here is that the current fixtures logic for bulk delete doesn't even care about the bucket and simply tries to find the given blobs in any bucket.
I wouldn't really put any effort into this tbh. I think we should probably rather look into just removing the fixture now that we have the Docker-based Minio tests and third-party tests.
The fixture seems completely redundant now ...

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

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

andrershov · 2019-03-29T12:34:51Z

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

If there is an IOException we do not proceed even if we have more DeleteRequests to be sent.
Previously when performing deletes - if one delete failed, we still were proceeding with the next delete requests.

Right, fixed this for S3 as well as the generic case now by catching and aggregating exceptions in the loop :)

andrershov · 2019-03-29T12:37:30Z

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

We no longer know what particular indices are not removed. We just log all indices, including those that are successful.
The same thing applies to deleteBlobs usage below.
Probably we can add this kind of information to IOException thrown by deleteBlobs?

Done in 9a13dadd0c1 :) We now aggregate all the exceptions. The IndexId that are printed here contain the name and the index snapshot id so with the information in the aggregate exception we can work out what index failed to clean up.

Do you know how AWS S3 works when you send a bulk request of 1000 and entry number 500 fails? Will it stop at entry 500 or will it try to delete all entries from the bulk?
Will S3 include information about each failed entry to the exception? (or just the first one?)

@andrershov it will try to delete all of the entries and given errors for all the ones that failed :)

original-brownbear · 2019-03-29T14:08:20Z

@andrershov thanks for taking a look! All points addressed and tests added in
ba3d485 and 2c3fd34 :)

andrershov

@original-brownbear mostly looks good. But before approving I want to understand how S3 deals with bulk requests in terms of exceptions when several deletions have failed.
I want to be sure that S3 will try to delete all the elements from the bulk even if some elements from the bulk have problems and that information about all failed elements will be included to the exception.

* Adds Bulk delete API to blob container * Implement bulk delete API for S3 * Adjust S3Fixture to accept both path styles for bulk deletes since the S3 SDK uses both during our ITs * Closes #40250

original-brownbear · 2019-04-01T13:20:20Z

@andrershov fixed the docs on deleteBlob (sorry for the force push, I was a little clumsy there and didn't realize what computer I was on at the time :D) and answered your S3 question :)

andrershov

LGTM

original-brownbear · 2019-04-01T13:55:38Z

@andrershov thanks
@ywelsch you're good with this one too?

ywelsch

Thanks @original-brownbear. Looking very good already. I've left some minor points

ywelsch · 2019-04-02T11:34:41Z

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

 class S3BlobContainer extends AbstractBlobContainer {

+    /**
+     * Maximum number of deletes in a {@link DeleteObjectsRequest}.


perhaps link to AWS docs here

ywelsch · 2019-04-02T11:52:10Z

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

    }

+    @Override
+    public void deleteBlobs(List<String> blobNames) throws IOException {


I think we should call this deleteBlobsIgnoringIfNotExists?

ywelsch · 2019-04-02T12:07:22Z

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

-                        "but failed to clean up its index folder.", metadata.name(), indexId), ioe);
+                    logger.warn(() ->
+                        new ParameterizedMessage(
+                            "[{}] indices [{}] are no longer part of any snapshots in the repository, " +


Second placeholder can be just {} instead of [{}] as it's a list that will render anyway with the brackets. Same issue in other places in this PR

ywelsch · 2019-04-02T12:10:10Z

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

-                                snapshotId, shardId, blobName), e);
-                        }
-                    }
+                final List<String> staleBlobs = blobs.keySet().stream()


perhaps call this orphanedBlobs?

ywelsch

LGTM

original-brownbear · 2019-04-02T20:45:44Z

thanks @ywelsch

…leniency * elastic/master: SQL: Fix deserialisation issue of TimeProcessor (elastic#40776) Improve GCS docs for using keystore (elastic#40605) Add Restore Operation to SnapshotResiliencyTests (elastic#40634) Small refactorings to analysis components (elastic#40745) SQL: Fix display size for DATE/DATETIME (elastic#40669) add HLRC protocol tests for transform state and stats (elastic#40766) Inline TransportReplAction#registerRequestHandlers (elastic#40762) remove experimental label from search_as_you_type documentation (elastic#40744) Remove some abstractions from `TransportReplicationAction` (elastic#40706) Upgrade to latest build scan plugin (elastic#40702) Use default memory lock setting in testing (elastic#40730) Add Bulk Delete Api to BlobStore (elastic#40322) Remove yaml skips older than 7.0 (elastic#40183) Docs: Move id in the java-api (elastic#40748)

* Adds Bulk delete API to blob container * Implement bulk delete API for S3 * Adjust S3Fixture to accept both path styles for bulk deletes since the S3 SDK uses both during our ITs * Closes #40250

* Implement Bulk Deletes for GCS Repository * 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

* 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

* Adds Bulk delete API to blob container * Implement bulk delete API for S3 * Adjust S3Fixture to accept both path styles for bulk deletes since the S3 SDK uses both during our ITs * Closes elastic#40250

* 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

Backport of elastic/elasticsearch#40322

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

original-brownbear requested review from andrershov and ywelsch March 21, 2019 19:48

andrershov suggested changes Mar 29, 2019

View reviewed changes

original-brownbear requested a review from andrershov March 29, 2019 14:08

andrershov suggested changes Apr 1, 2019

View reviewed changes

original-brownbear added 8 commits April 1, 2019 15:17

Add Bulk Delete Api to BlobStore

86e2648

* Adds Bulk delete API to blob container * Implement bulk delete API for S3 * Adjust S3Fixture to accept both path styles for bulk deletes since the S3 SDK uses both during our ITs * Closes #40250

another spot

704611c

CR comments

c8a768a

fix bulk delete exception handling

716c1c5

fix bulk delete exception handling

aa10b32

align fake s3 with real s3

8f2c20f

CR: fix wording

d6423d7

CR: fix wording

93aee4e

original-brownbear requested a review from andrershov April 1, 2019 13:20

andrershov approved these changes Apr 1, 2019

View reviewed changes

original-brownbear added 2 commits April 1, 2019 20:57

Merge remote-tracking branch 'elastic/master' into 40250

e592724

Merge remote-tracking branch 'elastic/master' into 40250

5f7f266

ywelsch suggested changes Apr 2, 2019

View reviewed changes

original-brownbear added 2 commits April 2, 2019 15:51

Merge remote-tracking branch 'elastic/master' into 40250

454bdea

CR comments

e7cb42e

original-brownbear requested a review from ywelsch April 2, 2019 14:11

ywelsch approved these changes Apr 2, 2019

View reviewed changes

original-brownbear merged commit b1e2cec into elastic:master Apr 2, 2019

original-brownbear deleted the 40250 branch April 2, 2019 20:46

original-brownbear added the backport pending label Apr 2, 2019

original-brownbear mentioned this pull request Apr 3, 2019

Async Snapshot Repository Deletes #40144

Merged

original-brownbear removed the backport pending label Apr 16, 2019

original-brownbear mentioned this pull request Apr 16, 2019

Add Bulk Delete Api to BlobStore (#40322) #41253

Merged

original-brownbear mentioned this pull request Apr 19, 2019

Implement Bulk Deletes for GCS Repository #41368

Merged

original-brownbear mentioned this pull request Apr 30, 2019

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

Merged

codebrain mentioned this pull request Aug 5, 2019

[meta] 7.2 Release elastic/elasticsearch-net#3980

Closed

37 tasks

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

6b2659b

Backport of elastic/elasticsearch#40322

mkleen mentioned this pull request May 26, 2020

Add Bulk Delete Api to BlobStore crate/crate#10000

Merged

5 tasks

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

d37420c

Backport of elastic/elasticsearch#40322

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

3e0aef4

Backport of elastic/elasticsearch#40322

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

9009e61

Backport of elastic/elasticsearch#40322

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

24b3169

Backport of elastic/elasticsearch#40322

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

3bf1a7f

Backport of elastic/elasticsearch#40322

mkleen added a commit to crate/crate that referenced this pull request May 26, 2020

Add Bulk Delete Api to BlobStore

bacaf18

Backport of elastic/elasticsearch#40322

mkleen added a commit to crate/crate that referenced this pull request May 27, 2020

Add Bulk Delete Api to BlobStore

8bdf476

Backport of elastic/elasticsearch#40322

mergify bot pushed a commit to crate/crate that referenced this pull request May 27, 2020

Add Bulk Delete Api to BlobStore

1ba21d7

Backport of elastic/elasticsearch#40322

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

Conversation

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

Uh oh!

Uh oh!

elasticmachine commented Mar 21, 2019

Uh oh!

original-brownbear commented Mar 21, 2019

Uh oh!

andrershov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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 Mar 29, 2019

Uh oh!

andrershov left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 1, 2019

Uh oh!

andrershov left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 1, 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!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

original-brownbear commented Mar 21, 2019 •

edited

Loading