Core: Enhance remove snapshots efficiency by executing them in bulk by ricardopereira33 · Pull Request #12670 · apache/iceberg

ricardopereira33 · 2025-03-27T19:05:07Z

This PR changes the remove-snapshots operation to run in bulk instead of single operations on the Rest Catalog server.

The remove-snapshots operation signature already accepts a list of snapshots IDs. However, each snapshot ID is stored in a dedicated Metadata object. This makes the operations not very efficient, causing performance issues, mentioned on: #12642.

The PR changes:

Rename MetadataUpdate class RemoveSnapshot to RemoveSnapshots.
Compute the entire snapshots IDs list and store them on the RemoveSnapshots instead of an individual snapshot ID.

amogh-jahagirdar · 2025-03-27T20:44:50Z

+  class RemoveSnapshots implements MetadataUpdate {
+    private final Set<Long> snapshotIds;

-    public RemoveSnapshot(long snapshotId) {
-      this.snapshotId = snapshotId;
+    public RemoveSnapshots(Set<Long> snapshotIds) {
+      this.snapshotIds = snapshotIds;
    }

-    public long snapshotId() {
-      return snapshotId;
+    public Set<Long> snapshotIds() {
+      return snapshotIds;
    }

    @Override
    public void applyTo(TableMetadata.Builder metadataBuilder) {
-      metadataBuilder.removeSnapshots(ImmutableSet.of(snapshotId));
+      metadataBuilder.removeSnapshots(snapshotIds);
    }


We should leaveRemoveSnapshot update as is since this is still a valid update type in the protocol. I think the solution should involve some sort of buffering of the snapshots to remove instead of eagerly rewriting (it's the eager rewriting of the current snapshots in the builder that leads to the n^2 behavior). Only at the end, once all the snapshots to remove have been accumulated, would we want to go ahead and rewrite. Let me take a deeper look and get back with a concrete solution

We should leaveRemoveSnapshot update as is since this is still a valid update type in the protocol.

@amogh-jahagirdar On the protocol, we already defined a list of snapshots to be removed... However, we just pass a single snapshot ID in the list. Why can't we change to have all snapshot IDs? Is there a reason for this to be like that?

@ricardopereira33 Ah you are completely right I think, I misread the protocol, RemoveSnapshots https://github.com/apache/iceberg/blame/main/open-api/rest-catalog-open-api.yaml#L2833 already operates on a list of snapshots. it's seems like the implementation should actually be more open. Currently looks like there's some strict validation that there's only 1 element https://github.com/apache/iceberg/blame/main/core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java#L559

I'm not sure the context on this. But fundamentally I think you are correct there's no protocol violation here. I'd need to see why this was implemented this way to begin with I also see this ToDo https://github.com/apache/iceberg/blame/main/core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java#L420

For general library compatibility though we probably need to keep this update type, potentially deprecating it in favor of the new one.

I can keep the RemoveSnapshot as a deprecated class 👍🏽
I'm bringing the contributors of that validation to the loop: @nastra @rdblue. Do you know why do we have this validation to restrict just 1 snapshot?

yes the RemoveSnapshot class needs to stay so that we don't break existing APIs, but should be marked deprecated (and refer to using the new API - we do it similarly in other places that are deprecated). Then you can introduce a new RemoveSnapshots API, which the TableMetadata Builder would call

ricardopereira33 · 2025-04-01T12:30:08Z

          .put(MetadataUpdate.RemovePartitionStatistics.class, REMOVE_PARTITION_STATISTICS)
          .put(MetadataUpdate.AddSnapshot.class, ADD_SNAPSHOT)
-          .put(MetadataUpdate.RemoveSnapshot.class, REMOVE_SNAPSHOTS)
+          .put(MetadataUpdate.RemoveSnapshots.class, REMOVE_SNAPSHOTS)


@nastra @amogh-jahagirdar In order to keep both RemoveSnapshot (deprecated) and RemoveSnapshots (new), we need to define a custom process when we are parsing the metadata updates from the JSON. Currently, RemoveSnapshot is pointing to the keyword remove-snapshots.

iceberg/core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java

Line 50 in e55f238

static final String REMOVE_SNAPSHOTS = "remove-snapshots";

To add a new MetadataUpdate option RemoveSnapshots, the alternatives are:

Reuse the same keyword, and add a custom condition to distinguish RemoveSnapshot and RemoveSnapshots.

Create a new keyword and have different parsing process for each metadata update.

However, the current protocol expects a list of values for the key remove-snapshots, so even if we replace the RemoveSnapshot completely, we are still using the protocol correctly. WDYT about this?

I'm not sure if this is what was meant by the first alternative presented but I think we could just have 2 cases in the interim (until RemoveSnapshot is removed after a deprecation cycle)

.put(MetadataUpdate.RemoveSnapshots.class, REMOVE_SNAPSHOTS) .put(MetadataUpdate.RemoveSnapshot.class, REMOVE_SNAPSHOTS)

When serializing the update, we'd be able to distinguish if it's removeSnapshot or RemoveSnapshots (instanceOf). you could update writeRemoveSnapshots to take in just a RemoveSnapshots instance, and for the RemoveSnapshot case, you'd construct a RemoveSnapshots passing in a list with a single snapshot element.

Then in the future once RemoveSnapshot is removed after deprecation, we could then just remove the additional handling for that case, and update the tests which have some expectations on the produced update types.

Let me know if that makes sense or not!

Reuse the same keyword, and add a custom condition to distinguish RemoveSnapshot and RemoveSnapshots

@amogh-jahagirdar yes, that was exactly what I meant for the first option 😆 I will prepare the changes today, thank you!

ricardopereira33 · 2025-04-03T09:25:50Z

+    MetadataUpdate metadataUpdate;
+    if (snapshotIds.size() == 1) {
+      Long snapshotId = Iterables.getOnlyElement(snapshotIds);
+      metadataUpdate = new MetadataUpdate.RemoveSnapshot(snapshotId);


To keep the same behaviour as before, and to not break existing tests.

nastra · 2025-04-03T11:57:14Z

@ricardopereira33 overall the approach LGTM, I just left a few minor comments that would be good to address

nastra

thanks @ricardopereira33 for fixing this. Let's also wait for @amogh-jahagirdar in case he has any feedback (we might also need to update the deprecation message depending on whether this will make it into 1.9.0)

amogh-jahagirdar

Approved with a nit. If we could address that along with the deprecation message to be 1.9.0, that'd be great

Thank you @ricardopereira33 for taking this on!

Edit: Ignore the nit, but we'll still need to update the deprecation message.

ricardopereira33 · 2025-04-03T16:51:02Z

@nastra @amogh-jahagirdar Pushed the change you requested. Thank you guys for the fast reviews and feedback! 💪🏽

amogh-jahagirdar · 2025-04-03T22:05:49Z

Thanks @ricardopereira33 and thanks @nastra for reviewing!

ajantha-bhat · 2025-05-09T06:26:58Z

      }

+      if (!snapshotIdsToRemove.isEmpty()) {
+        changes.add(new MetadataUpdate.RemoveSnapshots(snapshotIdsToRemove));


@nastra:

Question: Since we are sending RemoveSnapshots instead of RemoveSnapshot, isn't it a breaking change? Old server cannot understand this message. We need to update (about implement handling of new payload) the server to understand this new payload?

@ajantha-bhat the MetadataUpdateParser will still send a RemoveSnapshot for a single element for older clients

@ajantha-bhat check out this thread #12670 (comment)

Essentially the protocol was always RemoveSnapshots (servers were always handling this kind of message), and we're not deviating from that. RemoveSnapshot was just an internal in-memory representation. Let me know if that makes sense.

From the protocol level, it's backward compatible while since we have the following check
snapshotIds != null && snapshotIds.size() == 1, the new client against an old version which uses the old MetadataUpdateParser would still fail because of this check, right?

@sfc-gh-aixu: Thats what I observed too and started this thread. We had to update the server to new Iceberg version to make it work.

…n bulk (apache#12670)" This reverts commit 06f667a.

…anges Rather than in Bulk (#13100)

…hot Changes Rather than in Bulk (apache#13100)

github-actions Bot added the core label Mar 27, 2025

amogh-jahagirdar requested changes Mar 27, 2025

View reviewed changes

Core: Enhance remove snapshots efficiency by executing them in bulk

48cfacd

ricardopereira33 force-pushed the CORE-execute-remove-snapshots-in-bulk branch from e3110ee to 48cfacd Compare March 28, 2025 14:52

nastra reviewed Mar 31, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated

nastra reviewed Mar 31, 2025

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java

amogh-jahagirdar reviewed Mar 31, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated

Comment thread core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java

ricardopereira33 commented Apr 1, 2025

View reviewed changes

ricardopereira33 commented Apr 3, 2025

View reviewed changes

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdate.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdate.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java Outdated

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated

Core: Re-add RemoveSnapshot and combine it with the new RemoveSnapshots

cf6b395

ricardopereira33 force-pushed the CORE-execute-remove-snapshots-in-bulk branch from b7241e9 to cf6b395 Compare April 3, 2025 11:57

Core: Remove TODO comments

d363cf2

ricardopereira33 force-pushed the CORE-execute-remove-snapshots-in-bulk branch from bbc63cc to d363cf2 Compare April 3, 2025 12:55

nastra approved these changes Apr 3, 2025

View reviewed changes

nastra requested a review from amogh-jahagirdar April 3, 2025 13:56

amogh-jahagirdar approved these changes Apr 3, 2025

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java

nastra reviewed Apr 3, 2025

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/MetadataUpdate.java Outdated

Core: Change Iceberg version for deprecation of RemoveSnapshot

38b2626

amogh-jahagirdar merged commit 06f667a into apache:main Apr 3, 2025

ajantha-bhat reviewed May 9, 2025

View reviewed changes

aihuaxu added a commit to aihuaxu/iceberg that referenced this pull request May 19, 2025

Revert "Core: Enhance remove snapshots efficiency by executing them i…

68bdaa4

…n bulk (apache#12670)" This reverts commit 06f667a.

aihuaxu mentioned this pull request May 19, 2025

core: Send single snapshot to remove rather than in bulk #13100

Merged

RussellSpitzer pushed a commit that referenced this pull request May 21, 2025

Core: Revert #12670 to Temporarily Restore Sending Single Snapshot Ch…

f40208a

…anges Rather than in Bulk (#13100)

stevenzwu pushed a commit to stevenzwu/iceberg that referenced this pull request Jul 23, 2025

Core: Revert apache#12670 to Temporarily Restore Sending Single Snaps…

4735172

…hot Changes Rather than in Bulk (apache#13100)

Hugo-WB mentioned this pull request May 18, 2026

Core: cleanExpiredMetadata cleans unreferenced encryption keys #16395

Draft

Uh oh!

Conversation

ricardopereira33 commented Mar 27, 2025

Uh oh!

Uh oh!

amogh-jahagirdar Mar 27, 2025 • 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!

amogh-jahagirdar Mar 28, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Apr 1, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra commented Apr 3, 2025

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment • 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!

Uh oh!

Uh oh!

ricardopereira33 commented Apr 3, 2025

Uh oh!

amogh-jahagirdar commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar May 9, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amogh-jahagirdar Mar 27, 2025 •

edited

Loading

amogh-jahagirdar Mar 28, 2025 •

edited

Loading

amogh-jahagirdar Apr 1, 2025 •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar May 9, 2025 •

edited

Loading