Limit number of suppressed S3 deletion errors #123630

pxsalehi · 2025-02-27T18:07:09Z

We've seen this being an issue on 7.x although can happen on all
versions (I'm pretty sure this PR doesn't cleanly back-port to 7.x though).

Closes #123354

elasticsearchmachine · 2025-02-27T18:07:56Z

Hi @pxsalehi, I've created a changelog YAML for you.

pxsalehi · 2025-03-03T09:21:23Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

 */
 static final int MAX_BULK_DELETES = 1000;

+ static final int MAX_DELETE_EXCEPTIONS = 10;


I've picked this randomly more or less. Not sure what it should be. I'm not sure having more than this is useful.

👍 fine by me

pxsalehi · 2025-03-03T09:22:25Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

 return new S3BlobContainer(path, this);
 }

+ record DeletionExceptions(Exception exception, int count) {}


Ideally we could have added this behaviour to useOrSuppress but we'd need to each time fetch a copy of the suppressed errors to check the count, so I've done it only here.

TIL Throwable#getSuppressed is doing quite a bit more than just retrieving an array of exceptions. Still, this seems rather more complicated (and involves unnecessary allocations) vs just counting the exceptions in a separate AtomicInteger. We don't even need atomicity either, we could just have a class with two mutable fields to keep track of the suppression depth.

sure see 6b85c33.

elasticsearchmachine · 2025-03-03T09:23:25Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner · 2025-03-03T09:34:23Z

...pository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

+ var maxNoOfDeletions = 2 * S3BlobStore.MAX_DELETE_EXCEPTIONS;
+ var blobs = randomList(1, maxNoOfDeletions * maxBulkDeleteSize, ESTestCase::randomIdentifier);
+ blobContainer.deleteBlobsIgnoringIfNotExists(randomPurpose(), blobs.iterator());
+ fail("deletion should not succeed");


I'd prefer an expectThrows here (around just the deleteBlobsIgnoringIfNotExists call)

also done in 6b85c33.

DaveCTurner

Production code looks good, just one more thing about the new createBlobContainer override. I could be persuaded if you feel strongly about it.

DaveCTurner · 2025-03-03T11:15:36Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

 */
 static final int MAX_BULK_DELETES = 1000;

+ static final int MAX_DELETE_EXCEPTIONS = 10;


👍 fine by me

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

DaveCTurner · 2025-03-03T11:18:13Z

...pository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

+ final @Nullable TimeValue readTimeout,
+ final @Nullable Boolean disableChunkedEncoding,
+ final @Nullable ByteSizeValue bufferSize,
+ final @Nullable Integer maxBulkDeletes


I'd rather just have the one method rather than having to work out which variant is being called (and what the implied default params) are in each case. I know it adds a bunch of noise here but it'll be better in the long run.

yeah, mostly because of the noise and the fact that this way of passing these arguments is terrible to extend. Speaking of the long-run, how about replacing these with a parameter object?

Some kind of builder would seem in order here, but maybe that can be deferred to a followup. Or do that first, reducing the noise in this PR. But either way, let's not have two methods that do almost the same thing.

DaveCTurner

LGTM

pxsalehi · 2025-03-03T14:55:50Z

:qa:rolling-upgrade-legacy:v9.1.0#oldClusterTest is failing. Not sure even what that is, and can't find anything besides some gradle error in the build log. I'll merge main into this to pull in a few gradle/build related commits.

elasticsearchmachine · 2025-03-03T16:08:03Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts
❌	7.17	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123630

If all items fail to delete, the current warning log builds a huge string to include all of them (up to 1000). This PR limits the string to a length of 1000 characters. Relates: elastic#123630

ywangd · 2025-03-04T04:31:04Z

post-merge LGTM

I raised #123953 as a minor improvement for the relevant log message.

pxsalehi · 2025-03-04T09:47:17Z

Backporting to 7.17 seems a bit ugly. The code is too different and some settings the test relies on don't exit there. I'm gonna skip it. If you'd like to have it anyway, let me know.

If all items fail to delete, the current warning log builds a huge string to include all of them (up to 1000). This PR limits the string length to first 10 entries. Relates: #123630

If all items fail to delete, the current warning log builds a huge string to include all of them (up to 1000). This PR limits the string length to first 10 entries. Relates: elastic#123630 (cherry picked from commit a9432ba) # Conflicts: # modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java # modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

) * Limit the log line length for s3 deletion error (#123953) If all items fail to delete, the current warning log builds a huge string to include all of them (up to 1000). This PR limits the string length to first 10 entries. Relates: #123630 (cherry picked from commit a9432ba) # Conflicts: # modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java # modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java * fix backport

If all items fail to delete, the current warning log builds a huge string to include all of them (up to 1000). This PR limits the string length to first 10 entries. Relates: elastic#123630

Limit number of suppressed S3 deletion errors

80cbf72

pxsalehi added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Feb 27, 2025

elasticsearchmachine added the v9.1.0 label Feb 27, 2025

Update docs/changelog/123630.yaml

296fa18

Merge branch 'main' into ps250227-capDeletionSuppressedErrors

2a9f23b

pxsalehi added v8.19.0 v7.17.29 labels Mar 3, 2025

pxsalehi commented Mar 3, 2025

View reviewed changes

pxsalehi marked this pull request as ready for review March 3, 2025 09:23

pxsalehi requested review from DaveCTurner and ywangd March 3, 2025 09:23

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Mar 3, 2025

DaveCTurner reviewed Mar 3, 2025

View reviewed changes

address review comments

6b85c33

pxsalehi requested a review from DaveCTurner March 3, 2025 11:14

DaveCTurner reviewed Mar 3, 2025

View reviewed changes

address review comments

6662a8b

pxsalehi requested a review from DaveCTurner March 3, 2025 12:13

DaveCTurner approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into ps250227-capDeletionSuppressedErrors

9b3dac4

pxsalehi added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport Automatically create backport pull requests when merged labels Mar 3, 2025

Merge branch 'main' into ps250227-capDeletionSuppressedErrors

80d6bd4

elasticsearchmachine merged commit 113f0c1 into elastic:main Mar 3, 2025
17 checks passed

pxsalehi deleted the ps250227-capDeletionSuppressedErrors branch March 3, 2025 16:06

elasticsearchmachine added the backport pending label Mar 3, 2025

ywangd mentioned this pull request Mar 4, 2025

Limit the log line length for s3 deletion error #123953

Merged

pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Mar 4, 2025

Limit number of suppressed S3 deletion errors (elastic#123630)

311ccfc

pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Mar 4, 2025

Limit number of suppressed S3 deletion errors (elastic#123630)

508ddf0

pxsalehi mentioned this pull request Mar 4, 2025

[8.x] Limit number of suppressed S3 deletion errors (#123630) #123974

Merged

pxsalehi removed the v7.17.29 label Mar 4, 2025

pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Mar 4, 2025

Merge branch '8.x' into ps250303-cherryPick-elastic#123630

74e8eb3

elasticsearchmachine pushed a commit that referenced this pull request Mar 4, 2025

Limit number of suppressed S3 deletion errors (#123630) (#123974)

5c34bfb

pxsalehi removed the backport pending label Mar 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Limit number of suppressed S3 deletion errors #123630

Limit number of suppressed S3 deletion errors #123630

Uh oh!

pxsalehi commented Feb 27, 2025 •

edited

Loading

elasticsearchmachine commented Feb 27, 2025

pxsalehi Mar 3, 2025

DaveCTurner Mar 3, 2025

pxsalehi Mar 3, 2025 •

edited

Loading

DaveCTurner Mar 3, 2025

pxsalehi Mar 3, 2025

elasticsearchmachine commented Mar 3, 2025

DaveCTurner Mar 3, 2025

pxsalehi Mar 3, 2025

DaveCTurner left a comment

DaveCTurner Mar 3, 2025

Uh oh!

DaveCTurner Mar 3, 2025

pxsalehi Mar 3, 2025

DaveCTurner Mar 3, 2025

pxsalehi Mar 3, 2025

DaveCTurner left a comment

pxsalehi commented Mar 3, 2025

Uh oh!

elasticsearchmachine commented Mar 3, 2025

ywangd commented Mar 4, 2025

pxsalehi commented Mar 4, 2025 •

edited

Loading

Labels

4 participants

Limit number of suppressed S3 deletion errors #123630

Limit number of suppressed S3 deletion errors #123630

Uh oh!

Conversation

pxsalehi commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxsalehi Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

pxsalehi commented Mar 3, 2025

Uh oh!

elasticsearchmachine commented Mar 3, 2025

💔 Backport failed

ywangd commented Mar 4, 2025

pxsalehi commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Labels

4 participants

pxsalehi commented Feb 27, 2025 •

edited

Loading

pxsalehi Mar 3, 2025 •

edited

Loading

pxsalehi commented Mar 4, 2025 •

edited

Loading