From 102845f5ae66c4792ea1196a8c1977ddba3020df Mon Sep 17 00:00:00 2001 From: Armin Date: Tue, 30 Apr 2019 17:06:54 +0200 Subject: [PATCH 1/6] 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 --- .../gcs/GoogleCloudStorageBlobStore.java | 1 + .../repositories/s3/S3BlobContainer.java | 26 ++++++++++++------- .../blobstore/BlobStoreRepository.java | 12 ++++----- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java index d873a5cd29074..dab7c9627e6dc 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java @@ -343,6 +343,7 @@ public void error(StorageException exception) { if (e != null) { throw new IOException("Exception when deleting blobs [" + failedBlobs + "]", e); } + assert failedBlobs.isEmpty(); } private static String buildKey(String keyPath, String s) { diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java index 652fa6a36017e..038d94fae3f32 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java @@ -24,7 +24,9 @@ import com.amazonaws.services.s3.model.AmazonS3Exception; import com.amazonaws.services.s3.model.CompleteMultipartUploadRequest; import com.amazonaws.services.s3.model.DeleteObjectsRequest; +import com.amazonaws.services.s3.model.DeleteObjectsResult; import com.amazonaws.services.s3.model.InitiateMultipartUploadRequest; +import com.amazonaws.services.s3.model.MultiObjectDeleteException; import com.amazonaws.services.s3.model.ObjectListing; import com.amazonaws.services.s3.model.ObjectMetadata; import com.amazonaws.services.s3.model.PartETag; @@ -34,6 +36,7 @@ import com.amazonaws.services.s3.model.UploadPartRequest; import com.amazonaws.services.s3.model.UploadPartResult; import org.apache.lucene.util.SetOnce; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobMetaData; @@ -50,6 +53,8 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import static org.elasticsearch.repositories.s3.S3Repository.MAX_FILE_SIZE; import static org.elasticsearch.repositories.s3.S3Repository.MAX_FILE_SIZE_USING_MULTIPART; @@ -127,12 +132,13 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce if (blobNames.isEmpty()) { return; } + final Set outstanding = blobNames.stream().map(this::buildKey).collect(Collectors.toSet()); try (AmazonS3Reference clientReference = blobStore.clientReference()) { // S3 API only allows 1k blobs per delete so we split up the given blobs into requests of max. 1k deletes final List deleteRequests = new ArrayList<>(); final List partition = new ArrayList<>(); - for (String blob : blobNames) { - partition.add(buildKey(blob)); + for (String key : outstanding) { + partition.add(key); if (partition.size() == MAX_BULK_DELETES ) { deleteRequests.add(bulkDelete(blobStore.bucket(), partition)); partition.clear(); @@ -146,21 +152,23 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce for (DeleteObjectsRequest deleteRequest : deleteRequests) { try { clientReference.client().deleteObjects(deleteRequest); + outstanding.removeAll(partition); + } catch (MultiObjectDeleteException e) { + outstanding.removeAll( + e.getDeletedObjects().stream().map(DeleteObjectsResult.DeletedObject::getKey).collect(Collectors.toList())); + aex = ExceptionsHelper.useOrSuppress(aex, e); } catch (AmazonClientException e) { - if (aex == null) { - aex = e; - } else { - aex.addSuppressed(e); - } + aex = ExceptionsHelper.useOrSuppress(aex, e); } } if (aex != null) { throw aex; } }); - } catch (final AmazonClientException e) { - throw new IOException("Exception when deleting blobs [" + blobNames + "]", e); + } catch (Exception e) { + throw new IOException("Failed to delete blobs [" + outstanding + "]", e); } + assert outstanding.isEmpty(); } private static DeleteObjectsRequest bulkDelete(String bucket, List blobs) { diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index b7ca6224841e1..39a4011783a55 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -1000,8 +1000,8 @@ protected void finalize(final List snapshots, try { blobContainer.deleteBlobsIgnoringIfNotExists(blobNames); } catch (IOException e) { - logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to delete index blobs {} during finalization", - snapshotId, shardId, blobNames), e); + logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to delete index blobs during finalization", + snapshotId, shardId), e); throw e; } @@ -1016,8 +1016,8 @@ protected void finalize(final List snapshots, try { blobContainer.deleteBlobsIgnoringIfNotExists(indexBlobs); } catch (IOException e) { - logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to delete index blobs {} during finalization", - snapshotId, shardId, indexBlobs), e); + logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to delete index blobs during finalization", + snapshotId, shardId), e); throw e; } @@ -1029,8 +1029,8 @@ protected void finalize(final List snapshots, try { blobContainer.deleteBlobsIgnoringIfNotExists(orphanedBlobs); } catch (IOException e) { - logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to delete data blobs {} during finalization", - snapshotId, shardId, orphanedBlobs), e); + logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to delete data blobs during finalization", + snapshotId, shardId), e); } } catch (IOException e) { String message = "Failed to finalize " + reason + " with shard index [" + currentIndexGen + "]"; From 7d181b5e0ccb78b46fe7b10e2ff19a202e1b8db3 Mon Sep 17 00:00:00 2001 From: Armin Date: Wed, 1 May 2019 11:38:08 +0200 Subject: [PATCH 2/6] CR: fix outstanding collection --- .../org/elasticsearch/repositories/s3/S3BlobContainer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java index 038d94fae3f32..d4e5878d067af 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java @@ -152,7 +152,8 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce for (DeleteObjectsRequest deleteRequest : deleteRequests) { try { clientReference.client().deleteObjects(deleteRequest); - outstanding.removeAll(partition); + outstanding.removeAll( + deleteRequest.getKeys().stream().map(DeleteObjectsRequest.KeyVersion::getKey).collect(Collectors.toList())); } catch (MultiObjectDeleteException e) { outstanding.removeAll( e.getDeletedObjects().stream().map(DeleteObjectsResult.DeletedObject::getKey).collect(Collectors.toList())); From 7602394d3c037409b24280f4fe92f1586b84f415 Mon Sep 17 00:00:00 2001 From: Armin Date: Wed, 1 May 2019 14:24:58 +0200 Subject: [PATCH 3/6] CR: fix broken delete determination --- .../repositories/s3/S3BlobContainer.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java index d4e5878d067af..b7852c0e374e8 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java @@ -24,7 +24,6 @@ import com.amazonaws.services.s3.model.AmazonS3Exception; import com.amazonaws.services.s3.model.CompleteMultipartUploadRequest; import com.amazonaws.services.s3.model.DeleteObjectsRequest; -import com.amazonaws.services.s3.model.DeleteObjectsResult; import com.amazonaws.services.s3.model.InitiateMultipartUploadRequest; import com.amazonaws.services.s3.model.MultiObjectDeleteException; import com.amazonaws.services.s3.model.ObjectListing; @@ -150,13 +149,15 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce SocketAccess.doPrivilegedVoid(() -> { AmazonClientException aex = null; for (DeleteObjectsRequest deleteRequest : deleteRequests) { + List keysInRequest = + deleteRequest.getKeys().stream().map(DeleteObjectsRequest.KeyVersion::getKey).collect(Collectors.toList()); try { clientReference.client().deleteObjects(deleteRequest); - outstanding.removeAll( - deleteRequest.getKeys().stream().map(DeleteObjectsRequest.KeyVersion::getKey).collect(Collectors.toList())); + outstanding.removeAll(keysInRequest); } catch (MultiObjectDeleteException e) { - outstanding.removeAll( - e.getDeletedObjects().stream().map(DeleteObjectsResult.DeletedObject::getKey).collect(Collectors.toList())); + Set failedKeys = + e.getErrors().stream().map(MultiObjectDeleteException.DeleteError::getKey).collect(Collectors.toSet()); + outstanding.removeIf(key -> keysInRequest.contains(key) && failedKeys.contains(key) == false); aex = ExceptionsHelper.useOrSuppress(aex, e); } catch (AmazonClientException e) { aex = ExceptionsHelper.useOrSuppress(aex, e); From 214d80aa582978a5c0cc87465a89752dd715a174 Mon Sep 17 00:00:00 2001 From: Armin Date: Wed, 1 May 2019 14:28:45 +0200 Subject: [PATCH 4/6] CR: fix broken delete determination --- .../elasticsearch/repositories/s3/S3BlobContainer.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java index b7852c0e374e8..e84c8d48426cf 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java @@ -151,15 +151,15 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce for (DeleteObjectsRequest deleteRequest : deleteRequests) { List keysInRequest = deleteRequest.getKeys().stream().map(DeleteObjectsRequest.KeyVersion::getKey).collect(Collectors.toList()); + outstanding.removeAll(keysInRequest); try { clientReference.client().deleteObjects(deleteRequest); - outstanding.removeAll(keysInRequest); } catch (MultiObjectDeleteException e) { - Set failedKeys = - e.getErrors().stream().map(MultiObjectDeleteException.DeleteError::getKey).collect(Collectors.toSet()); - outstanding.removeIf(key -> keysInRequest.contains(key) && failedKeys.contains(key) == false); + outstanding.addAll( + e.getErrors().stream().map(MultiObjectDeleteException.DeleteError::getKey).collect(Collectors.toSet())); aex = ExceptionsHelper.useOrSuppress(aex, e); } catch (AmazonClientException e) { + outstanding.addAll(keysInRequest); aex = ExceptionsHelper.useOrSuppress(aex, e); } } From 1fc3f2db01c6ee63ce6cfe818b4073e792bb2266 Mon Sep 17 00:00:00 2001 From: Armin Date: Wed, 1 May 2019 14:31:01 +0200 Subject: [PATCH 5/6] CR: fix broken delete determination --- .../org/elasticsearch/repositories/s3/S3BlobContainer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java index e84c8d48426cf..bdb9c959a9a46 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java @@ -151,15 +151,15 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce for (DeleteObjectsRequest deleteRequest : deleteRequests) { List keysInRequest = deleteRequest.getKeys().stream().map(DeleteObjectsRequest.KeyVersion::getKey).collect(Collectors.toList()); - outstanding.removeAll(keysInRequest); try { clientReference.client().deleteObjects(deleteRequest); + outstanding.removeAll(keysInRequest); } catch (MultiObjectDeleteException e) { + outstanding.removeAll(keysInRequest); outstanding.addAll( e.getErrors().stream().map(MultiObjectDeleteException.DeleteError::getKey).collect(Collectors.toSet())); aex = ExceptionsHelper.useOrSuppress(aex, e); } catch (AmazonClientException e) { - outstanding.addAll(keysInRequest); aex = ExceptionsHelper.useOrSuppress(aex, e); } } From 5a76eb8b97d8f3ce09333b7c53e87e093e96a62e Mon Sep 17 00:00:00 2001 From: Armin Date: Wed, 1 May 2019 17:35:04 +0200 Subject: [PATCH 6/6] CR: add comment --- .../org/elasticsearch/repositories/s3/S3BlobContainer.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java index bdb9c959a9a46..c057d330da540 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java @@ -155,11 +155,15 @@ public void deleteBlobsIgnoringIfNotExists(List blobNames) throws IOExce clientReference.client().deleteObjects(deleteRequest); outstanding.removeAll(keysInRequest); } catch (MultiObjectDeleteException e) { + // We are sending quiet mode requests so we can't use the deleted keys entry on the exception and instead + // first remove all keys that were sent in the request and then add back those that ran into an exception. outstanding.removeAll(keysInRequest); outstanding.addAll( e.getErrors().stream().map(MultiObjectDeleteException.DeleteError::getKey).collect(Collectors.toSet())); aex = ExceptionsHelper.useOrSuppress(aex, e); } catch (AmazonClientException e) { + // The AWS client threw any unexpected exception and did not execute the request at all so we do not + // remove any keys from the outstanding deletes set. aex = ExceptionsHelper.useOrSuppress(aex, e); } }