Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Exists Check from S3 Repository Deletes #40931

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Remove Exists Check from S3 Repository Deletes #40931

merged 2 commits into from
Apr 25, 2019

Conversation

original-brownbear
Copy link
Member

  • The check doesn't add much if anything practically, since the S3 repository is eventually consistent and we only log the non-existence of a blob anyway
    • We don't do the check on writes for this very reason and documented it as such
    • Removing the check saves one API call per single delete speeding up the deletion process and lowering costs

@original-brownbear
Remove Exists Check from S3 Repository Deletes
9d15176 
* The check doesn't add much if anything practically, since the S3 repository is eventually consistent and we only log the non-existence of a blob anyway
  * We don't do the check on writes for this very reason and documented it as such
  * Removing the check saves one API call per single delete speeding up the deletion process and lowering costs
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

original-brownbear
original-brownbear commented Apr 7, 2019
View reviewed changes
server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java
@@ -120,9 +120,7 @@ default void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOExc
IOException ioe = null;
for (String blobName : blobNames) {
try {
deleteBlob(blobName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw this and figured I'd dry this up here, no need to have duplicate logic for suppressing that exception.

original-brownbear
original-brownbear commented Apr 7, 2019
View reviewed changes
...repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java
@@ -65,6 +65,11 @@ protected BlobStore newBlobStore() {
return randomMockS3BlobStore();
}

@Override
public void testDeleteBlob() {
assumeFalse("not implemented because of S3's weak consistency model", true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of the test disabled here is testing the existence check before delete. Since it's running against a mock S3 implementation, it's completely superfluous anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to serve it for documentation purposes, you'd better give a different name. testDeleteFailsWithNoSuchFileException, for example.

andrershov
andrershov approved these changes Apr 25, 2019
View reviewed changes
Copy link
Contributor

@andrershov andrershov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left one comment requesting test renaming. No need for the second pass.

...repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java
@@ -65,6 +65,11 @@ protected BlobStore newBlobStore() {
return randomMockS3BlobStore();
}

@Override
public void testDeleteBlob() {
assumeFalse("not implemented because of S3's weak consistency model", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to serve it for documentation purposes, you'd better give a different name. testDeleteFailsWithNoSuchFileException, for example.

@original-brownbear
Copy link
Member Author

thanks @andrershov !

@original-brownbear original-brownbear merged commit 180deaa into elastic:master Apr 25, 2019
@original-brownbear original-brownbear deleted the always-force-delete branch April 25, 2019 14:18
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Apr 25, 2019
@original-brownbear
Remove Exists Check from S3 Repository Deletes (elastic#40931)
2de80fd 
* The check doesn't add much if anything practically, since the S3 repository is eventually consistent and we only log the non-existence of a blob anyway
  * We don't do the check on writes for this very reason and documented it as such
  * Removing the check saves one API call per single delete speeding up the deletion process and lowering costs
@original-brownbear original-brownbear mentioned this pull request Apr 25, 2019
Merged
original-brownbear added a commit that referenced this pull request Apr 25, 2019
@original-brownbear
Remove Exists Check from S3 Repository Deletes (#40931) (#41534)
23b3741 
* The check doesn't add much if anything practically, since the S3 repository is eventually consistent and we only log the non-existence of a blob anyway
  * We don't do the check on writes for this very reason and documented it as such
  * Removing the check saves one API call per single delete speeding up the deletion process and lowering costs
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 26, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into java-11-collection…
62d7b7f 
…s-in-all-the-places

* elastic/master: (70 commits)
  Remove experimental label froms script_score query (elastic#41572)
  [Ml-Dataframe] Update URLs in Data frame client java doc (elastic#41539)
  Reenable bwc Tests in master (elastic#41540)
  Fix search_as_you_type's sub-fields to pick their names from the full path of the root field (elastic#41541)
  Remove search analyzers from DocumentFieldMappers (elastic#41484)
  Update community client and integration docs (elastic#41513)
  Remove Version.V_6_x_x constants use in security (elastic#41185)
  Mute testDriverConfigurationWithSSLInURL
  Remove dedicated SSL network write buffer (elastic#41283)
  Disable max score optimization for queries with unbounded max scores (elastic#41361)
  [DOCS] Explicitly set section IDs for Asciidoctor migration (elastic#41547)
  [ML] add multi node integ tests for data frames (elastic#41508)
  [Docs] Fix common word repetitions (elastic#39703)
  [DOCS] Note TESTRESPONSE can't be used immediately after TESTSETUP (elastic#41542)
  Update configuring-ldap-realm.asciidoc (elastic#40427)
  Fixed very small typo in date (elastic#41398)
  Refactor GeoHashUtils (elastic#40869)
  Make 0 as invalid value for `min_children` in `has_child` query (elastic#41347)
  field_caps: adapt bwc version after backport (elastic#41427)
  Remove Exists Check from S3 Repository Deletes (elastic#40931)
  ...
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this pull request May 2, 2019
@original-brownbear @akhil10x5
Remove Exists Check from S3 Repository Deletes (elastic#40931)
3a7f509 
* The check doesn't add much if anything practically, since the S3 repository is eventually consistent and we only log the non-existence of a blob anyway
  * We don't do the check on writes for this very reason and documented it as such
  * Removing the check saves one API call per single delete speeding up the deletion process and lowering costs
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@original-brownbear
Remove Exists Check from S3 Repository Deletes (elastic#40931)
17fed7a 
* The check doesn't add much if anything practically, since the S3 repository is eventually consistent and we only log the non-existence of a blob anyway
  * We don't do the check on writes for this very reason and documented it as such
  * Removing the check saves one API call per single delete speeding up the deletion process and lowering costs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrershov andrershov andrershov approved these changes

@ywelsch ywelsch Awaiting requested review from ywelsch

Assignees
No one assigned
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@original-brownbear @elasticmachine @andrershov @jakelandis