Core: Update Remove Snapshots procedure for branching and tagging #4578

amogh-jahagirdar · 2022-04-18T04:45:31Z

This PR contains the updated expire snapshots procedure for branching and tagging. Implements the algorithm outlined here https://github.com/apache/iceberg/blob/master/format/spec.md#snapshot-retention-policy.

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

amogh-jahagirdar · 2022-04-18T04:52:21Z

Still missing unit tests on this but wanted to get feedback on the code while I add the tests.

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

amogh-jahagirdar · 2022-05-31T04:06:36Z

Ok, after stepping through the debugger I think I see what's going on. The failing incremental scan test can't find a staged snapshot because the updated expiration logic actually will remove it.

Expire snapshots previously would retain staged snapshots so long as it's not outside of the "expire older than" age. However, in this updated logic, we're not considering that for unreferenced staged snapshots. https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L173
This logic will retain any snapshot within the expire older than, which I think the updated procedure should do for any snapshot which is not part of any branches.

Currently age retention is only applied to snapshots which are part of branches. I think we definitely want to preserve the existing behavior to avoid deleting staged snapshots that people may expect should be retained based on existing behavior, so I will add logic for that.

@rdblue @jackye1995 let me know if you see any other cases around this.

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

amogh-jahagirdar · 2022-05-31T06:33:53Z

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

+    return retainedRefs;
+  }
+
+  private Set<Long> computeDanglingSnapshotsToRetain(Map<SnapshotRef, Set<Long>> branchSnapshots) {


@rdblue Let me know what you think of this logic to handle the case of unreferenced staged snapshots which should still be retained based on the default expiration age. I reorganized the code to minimize the computation of branch snapshots. I also should add a test for this case.

This should be updated to be any unreferenced snapshot not just branch history. If there's a staged snapshot which is tagged, that should not be counted as a dangling snapshot to retain.

Edit: Updated to handle this case and added a test

rdblue · 2022-05-31T22:01:29Z

@amogh-jahagirdar, I agree that we shouldn't remove unreferenced snapshots until they are older than the default expiration timestamp. Good catch!

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

rdblue · 2022-06-01T02:26:42Z

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

+    Map<SnapshotRef, Set<Long>> refSnapshots = computeRefSnapshots(base.refs().values());
+
+    // Identify unreferenced snapshots which should be retained
+    Set<Long> unreferencedSnapshotsToRetain = computeUnreferencedSnapshotsToRetain(refSnapshots);


I think this should be passed a single set of retained snapshots and look for any others, rather than using a complex map. Just do this analysis last, after you know all the other snapshots that are retained for other reasons.

Yeah I'm still thinking if there's a way to simplify this.

I think we'll need the structure for maintaining what are the referenced snapshots ("referenced" meaning for a branch all it's ancestors, and for a tag, the snapshot it refers to). I don't think we can construct the snapshot retention set and then after that look through all the table's snapshots , and correctly derive which snapshots are the unreferenced snapshots which should still be retained based on expiration age.

The reason is some of the table's snapshots may no longer need to be retained due to branch retention policy but if we go through those snapshots and they're still within defaultExpirationAge we'd retain them which I don't think we want since the branch has it's own lifecycle. I think if a snapshot was part of a branch which expired due to it's own retention policy then we don't want to retain it on the basis of the defaultExpireOlderThan.

In other words, by just using the retention set and the existing table snapshots I don't think we can differentiate between a snapshot which should actually be removed based on branch retention or if it's a snapshot which was never part of any reference.

I think we need to be able to identify what are the snapshots which are not part of any branch lineage or are tagged and for that we need a set of all the snapshots which encapsulate all snapshots which are currently part of a reference . We do the grouping so that it can be reused when determining which branch snapshots to keep (so that the ancestors don't have to be iterated through again). Let me know what you think @rdblue !

I agree with you on the behavior, but I think there's a simpler way to implement it. I went ahead and added it in amogh-jahagirdar#1, and fixed some tests. Can you review and merge that PR so we can get this in?

Merged! I think the new approach simplifies the code. what I was originally going for was not having to traverse the branch ancestors multiple times, which is why the map was retained but that may be overkill. Thanks for adding more tests!

…ng (#1) Cleanup remaining issues with snapshot expiration

rdblue · 2022-06-14T14:42:17Z

Thanks, @amogh-jahagirdar!

namrathamyske · 2022-07-06T05:50:50Z

@amogh-jahagirdar @rdblue With this PR, the breaking change is to set cleanExpiredFiles to false if the table has more than one ref. Is this Issue addressed anywhere?

amogh-jahagirdar · 2022-07-07T03:16:18Z

@namrathamyske

With this PR, the breaking change is to set cleanExpiredFiles to false if the table has more than one ref.

No, the operation will fail if cleanExpiredFiles is set and there are multiple retained refs. It shouldn't break any existing cases prior to the addition of branching/tagging and should be backwards compatible but please report in case you ran into any issues!

See this comment . To do a correct removal of files with multiple references, a reachability analysis needs to be done which requires quite a bit more implementation which is why for this PR we opted for just failing if cleanExpiredFiles is set and there are multiple branches.

namrathamyske · 2022-07-08T16:30:30Z

@amogh-jahagirdar Thanks for the info. Earlier i never set cleanExpiredFiles. It defaults to True. So i did not get any exception when I try expire snapshots. Now i need to explicitly set cleanExpiredFiles as False when I have multiple refs.

)

zinking · 2022-10-12T07:49:57Z

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

        .run(item -> {
          TableMetadata updated = internalApply();
+          if (cleanExpiredFiles && updated.refs().size() > 1) {
+            throw new UnsupportedOperationException("Cannot incrementally clean files for tables with more than 1 ref");


@amogh-jahagirdar I have a simple use case blocked by this line.

I am tagging one of the snapshots on the mainline as never expire, but as soon as I tag it, the normal expire procedure (expire with clean) stopped working, as there are two refs now.

is the constraint correct here ? because in my scenario, I am merely indicating SKIP this snapshot, with other terms stay the same.

I think, if there's only one branch main, and tags are all on main, it is safe to carry on.
is that correct ? otherwise when is the expired file supposed to be cleanedUp ? through removeOrphan?

github-actions bot added the core label Apr 18, 2022

amogh-jahagirdar commented Apr 18, 2022

View reviewed changes