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

Cleanup Bulk Delete Exception Logging #41693

Merged
merged 9 commits into from
May 6, 2019
Merged

Cleanup Bulk Delete Exception Logging #41693

merged 9 commits into from
May 6, 2019

Conversation

original-brownbear
Copy link
Member

@original-brownbear
Cleanup Bulk Delete Exception Logging
102845f 
* 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
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@original-brownbear
Copy link
Member Author

link relevant discussion #41368 (comment)

andrershov
andrershov suggested changes May 1, 2019
View reviewed changes
plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java Outdated
@@ -146,21 +152,23 @@ public void deleteBlobsIgnoringIfNotExists(List<String> blobNames) throws IOExce
for (DeleteObjectsRequest deleteRequest : deleteRequests) {
try {
clientReference.client().deleteObjects(deleteRequest);
outstanding.removeAll(partition);
Copy link
Contributor

Choose a reason for hiding this comment

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

You're removing "partition" here which is always the last partition, not the partition that is deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂ fixed :) sorry about that. Now using the keys from the delete request here (can't use the ones from the response, since it won't contain already missing blob names).

Copy link
Contributor

Choose a reason for hiding this comment

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

@original-brownbear Are you sure that missing blobs won't be reported in the response?
AWS S3 docs (https://docs.aws.amazon.com/en_us/AmazonS3/latest/API/multiobjectdeleteapi.html) say:

Note that, if the object specified in the request is not found, Amazon S3 returns the result as deleted.

If they're not reported, they won't be reported in MultiObjectDeleteException exception as well, and it would mean that exception handling logic is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

See here https://github.com/elastic/elasticsearch/pull/41693/files#diff-e915a7a019920d21471e0f39c435aa16R176. We have the quiet (non-verbose) option activated, so only keys that encounter an error are included. So for exceptions we're good imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

@original-brownbear sorry, I don't get it. If we're using quiet mode and only errors are reported it means we can not fetch the list of successfully deleted objects from MultiObjectDeleteException as well?
From MultiObjectDeleteException.getDeletedObjects JavaDoc

Returns the list of successfully deleted objects from this request. If
     * {@link DeleteObjectsRequest#getQuiet()} is true, only error responses
     * will be returned from s3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah fair point :D Added some more logic to account for this case now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@original-brownbear good. Now, why we mess up with outstanding and performing remove/add to it? Why not just to add errors to the failedBlobs set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically just to cover running into some other runtime exception in the loop that is then caught in https://github.com/elastic/elasticsearch/pull/41693/files#diff-e915a7a019920d21471e0f39c435aa16R171 (just in case I guess ... not sure how likely/possible it is running into that).

Copy link
Contributor

Choose a reason for hiding this comment

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

@original-brownbear do you mean running into AmazonClientException? Ok, I see your point. Can you please add the comment to the method describing the design decision because of quiet mode and generic AmazonClientException?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc
Jenkins run elasticsearch-ci/default-distro

@original-brownbear
Copy link
Member Author

@andrershov ping :)

andrershov
andrershov approved these changes May 6, 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.

@original-brownbear sorry for the delay, now LGTM

@original-brownbear
Copy link
Member Author

@andrershov np + thanks for reviewing!

@original-brownbear original-brownbear merged commit 59eeaa0 into elastic:master May 6, 2019
@original-brownbear original-brownbear deleted the adjust-bulk-logging-s3-ex branch May 6, 2019 14:59
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 7, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into jdk-13-ci
918f028 
* elastic/master: (414 commits)
  Add tasks to build Docker build context artifacts (elastic#41819)
  Replace more uses of immutable map builder (elastic#41823)
  Force selection of calendar or fixed intervals in date histo agg (elastic#33727)
  Switch run task to use real distro (elastic#41590)
  Clarify that path_match also considers object fields. (elastic#41658)
  [DOCS] remove 'es.scripting.update.ctx_in_params' system property for 7.0 (elastic#41643)
  Clarify _doc is a permanent part of certain document APIs. (elastic#41727)
  Remove the jdk directory to save space on bwc tests (elastic#41743)
  Fix full text queries test that start with now (elastic#41854)
  Remove `nonApplicationWrite` from `SSLDriver` (elastic#41829)
  SQL: [Docs] Add example for custom bucketing with CASE (elastic#41787)
  Cleanup Bulk Delete Exception Logging (elastic#41693)
  [DOCS] Rewrite `term` query docs for new format (elastic#41498)
  Mute PermissionsIT#testWhen[...]ByILMPolicy (elastic#41858)
  ReadOnlyEngine assertion fix (elastic#41842)
  [ML] addresses preview bug, and adds check to PUT (elastic#41803)
  Fix javadoc in WrapperQueryBuilder
  Testsclusters use seprate configurations per version (elastic#41504)
  Skip explain fetch sub phase when request holds only suggestions (elastic#41739)
  remove unused import
  ...
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@original-brownbear
Cleanup Bulk Delete Exception Logging (elastic#41693)
f1a11f2 
* 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
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request May 28, 2019
@original-brownbear
Cleanup Bulk Delete Exception Logging (elastic#41693)
1d0eabf 
* 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
@original-brownbear original-brownbear mentioned this pull request May 28, 2019
Merged
original-brownbear added a commit that referenced this pull request May 28, 2019
@original-brownbear
Cleanup Bulk Delete Exception Logging (#41693) (#42606)
116b050 
* 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
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.3.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