Limit heap used tracking `IndexMetadata` deletions by DaveCTurner · Pull Request #140394 · elastic/elasticsearch

DaveCTurner · 2026-01-08T19:06:07Z

In #133558 we imposed a limit on the heap used to keep track of
shard-level blobs to clean up after the commit of a snapshot deletion.
This commit makes use of the same mechanism to track IndexMetadata
blobs for future deletion.

Closes #140018

In elastic#133558 we imposed a limit on the heap used to keep track of shard-level blobs to clean up after the commit of a snapshot deletion. This commit makes use of the same mechanism to track `IndexMetadata` blobs for future deletion. Closes elastic#140018

elasticsearchmachine · 2026-01-08T19:06:33Z

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

elasticsearchmachine · 2026-01-08T19:06:34Z

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

DaveCTurner

This change is a little noisy because I've renamed things to avoid referring to shard-level things now that they also include index-level ones.

DaveCTurner · 2026-01-08T19:16:13Z

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

        private void writeUpdatedShardMetadataAndComputeDeletes(ActionListener<Void> listener) {
            // noinspection resource -- closed safely at the end of the iteration
-            final var listeners = new RefCountingListener(listener);
+            final var listeners = new RefCountingListener(ActionListener.runBefore(listener, this::recordUnreferencedIndicesMetadata));


NB moves the computation here, before we commit the new RepositoryData blob, because this is the right place to enhance it in future with the idea to list the actual blobs to be deleted. Even before doing that, if we compute these blobs here we can be sure there's no other snapshot deletions ongoing so we don't have to worry about concurrent memory usage.

DaveCTurner · 2026-01-08T19:21:26Z

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

+                    final var indexPath = indexPath(indexId).buildAsString();
+                    assert indexPath.startsWith(basePath);
+                    final var truncatedIndexPath = indexPath.substring(basePathLen);


Previously we were adding the full path including basePath to each blob and then removing it again later on, but we can cut that out here.

DaveCTurner · 2026-01-08T19:22:06Z

server/src/main/java/org/elasticsearch/repositories/RepositoryData.java

-        final Map<IndexId, Collection<String>> toRemove = new HashMap<>();
-        while (indicesForSnapshot.hasNext()) {
-            final var indexId = indicesForSnapshot.next();
+        return Iterators.flatMap(indicesToUpdateAfterRemovingSnapshot(snapshotIds), indexId -> {


This is the significant change: make this an Iterator so that we don't need to materialize more than one index's worth of blobs at once.

…tadata-usage

joshua-adams-1

LGTM in general, just a few nits

joshua-adams-1 · 2026-01-09T14:16:34Z

server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java

            .get();

        final var repo = setupRepo();
-        try (var shardBlobsToDelete = repo.new ShardBlobsToDelete()) {


[Nit] The test name and javadoc need updating since it is no longer ShardBlobsToDelete

joshua-adams-1 · 2026-01-09T14:20:41Z

server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java


        final var repo = setupRepo();
-        try (var shardBlobsToDelete = repo.new ShardBlobsToDelete()) {
+        try (var shardBlobsToDelete = repo.new BlobsToDelete()) {


[Nit] Should this variable still be called shardBlobsToDelete?

joshua-adams-1 · 2026-01-09T14:20:44Z

server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java

-                    final var shardId = between(1, 30);
-                    final var shardGeneration = new ShardGeneration(randomUUID());
+
                    // Always write at least one blob, guaranteeing that the shardDeleteResults stream increases in size


[Nit] AFAICT from the git diff, the final var blobsToDelete variable this comment belongs to has moved to line 798 so can this comment go there too? (I also see a similar need for it above line 814). Also I think shardDeleteResults needs renaming

++ fixed; it's actually not important that we always write one blob because we always e.g. write the IndexId

joshua-adams-1 · 2026-01-09T14:22:17Z

server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java

+                    final List<String> blobsToDelete;
+                    final CheckedRunnable<Exception> addResult;
+                    final UnaryOperator<String> blobNameOperator;
+                    if (randomBoolean()) {


Perhaps a one line comment here explaining what is happening so that the next person who reads the test understands we're testing two branches. Or, and probably my preferred option, would be updating the Javadoc comment above the test

…tadata-usage

joshua-adams-1

LGTM!

In elastic#133558 we imposed a limit on the heap used to keep track of shard-level blobs to clean up after the commit of a snapshot deletion. This commit makes use of the same mechanism to track `IndexMetadata` blobs for future deletion. Closes elastic#140018

DaveCTurner requested a review from joshua-adams-1 January 8, 2026 19:06

DaveCTurner added >bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v9.4.0 labels Jan 8, 2026

elasticsearchmachine added the Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. label Jan 8, 2026

Update docs/changelog/140394.yaml

5ff3fdc

DaveCTurner commented Jan 8, 2026

View reviewed changes

DaveCTurner added 2 commits January 9, 2026 09:32

Merge branch 'main' into 2026/01/08/snapshot-deletions-bound-index-me…

897c8bb

…tadata-usage

Merge branch 'main' into 2026/01/08/snapshot-deletions-bound-index-me…

6fa2f56

…tadata-usage

joshua-adams-1 reviewed Jan 9, 2026

View reviewed changes

DaveCTurner added 2 commits January 12, 2026 09:20

Merge branch 'main' into 2026/01/08/snapshot-deletions-bound-index-me…

c845a9b

…tadata-usage

Rename test/vars & fix comments

ba80666

DaveCTurner enabled auto-merge (squash) January 12, 2026 09:34

Assert addResult never fails

ad19daf

joshua-adams-1 approved these changes Jan 12, 2026

View reviewed changes

DaveCTurner merged commit e3caa46 into elastic:main Jan 12, 2026
35 checks passed

DaveCTurner deleted the 2026/01/08/snapshot-deletions-bound-index-metadata-usage branch January 12, 2026 12:58

Conversation

DaveCTurner commented Jan 8, 2026

Uh oh!

elasticsearchmachine commented Jan 8, 2026

Uh oh!

elasticsearchmachine commented Jan 8, 2026

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

joshua-adams-1 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!

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!

joshua-adams-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DaveCTurner Jan 8, 2026 •

edited

Loading