amogh-jahagirdar commented on code in PR #4578:
URL: https://github.com/apache/iceberg/pull/4578#discussion_r863922055
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -163,21 +188,100 @@ private TableMetadata internalApply() {
this.base = ops.refresh();
Set<Long> idsToRetain = Sets.newHashSet();
- List<Long> ancestorIds = SnapshotUtil.ancestorIds(base.currentSnapshot(),
base::snapshot);
- if (minNumSnapshots >= ancestorIds.size()) {
+ // Identify refs that should be removed
+ Set<String> expiredRefs = computeExpiredRefs(base.refs());
+ Set<Long> idsOfRetainedRefs = base.refs().entrySet().stream()
+ .filter(ref -> !expiredRefs.contains(ref.getKey()))
+ .map(refEntry -> refEntry.getValue().snapshotId())
+ .collect(Collectors.toSet());
+
+ for (Long idToRemove : idsToRemove) {
+ Preconditions.checkArgument(!idsOfRetainedRefs.contains(idToRemove),
+ "Cannot expire %s. There is a reference to it", idToRemove);
+ }
+
+ idsToRetain.addAll(idsOfRetainedRefs);
+
+ List<Map.Entry<String, SnapshotRef>> branches =
base.refs().entrySet().stream()
+ .filter(refEntry -> refEntry.getValue().isBranch())
+ .collect(Collectors.toList());
+
+ Set<Long> branchSnapshotsToRetain =
computeAllBranchSnapshotsToRetain(branches);
+ idsToRetain.addAll(branchSnapshotsToRetain);
+ TableMetadata.Builder updatedMetaBuilder = TableMetadata.buildFrom(base);
+ idsToRemove.addAll(base.snapshots().stream().map(Snapshot::snapshotId)
+ .filter(snapshot ->
!idsToRetain.contains(snapshot)).collect(Collectors.toSet()));
+ updatedMetaBuilder.removeSnapshots(idsToRemove);
+ expiredRefs.forEach(updatedMetaBuilder::removeRef);
+ TableMetadata updatedMetadata = updatedMetaBuilder.build();
+ List<Snapshot> updateSnapshots = updatedMetadata.snapshots();
+ List<Snapshot> baseSnapshots = base.snapshots();
+ boolean updated = updateSnapshots.size() != baseSnapshots.size() ||
expiredRefs.size() > 0;
+ return updated ? updatedMetadata : base;
+ }
+
+ private Set<Long> computeAllBranchSnapshotsToRetain(List<Map.Entry<String,
SnapshotRef>> branches) {
+ Set<Long> branchSnapshotsToRetain = Sets.newHashSet();
+ for (Map.Entry<String, SnapshotRef> branchEntry : branches) {
+ String name = branchEntry.getKey();
+ SnapshotRef branch = branchEntry.getValue();
+ Long expireSnapshotsOlderThan;
+ Integer minSnapshotsToKeep;
+ if (name.equals(SnapshotRef.MAIN_BRANCH) || branch.maxSnapshotAgeMs() ==
null) {
+ expireSnapshotsOlderThan = this.expireOlderThan;
+ } else {
+ expireSnapshotsOlderThan = now - branch.maxSnapshotAgeMs();
+ }
+
+ if (name.equals(SnapshotRef.MAIN_BRANCH) || branch.minSnapshotsToKeep()
== null) {
+ minSnapshotsToKeep = this.minNumSnapshots;
+ } else {
+ minSnapshotsToKeep = branch.minSnapshotsToKeep();
+ }
+
+ branchSnapshotsToRetain.addAll(
+ computeBranchSnapshotsToRetain(branch, expireSnapshotsOlderThan,
minSnapshotsToKeep));
+ }
+ return branchSnapshotsToRetain;
+ }
+
+ private Set<String> computeExpiredRefs(Map<String, SnapshotRef> refs) {
+ Set<String> expiredRefs = Sets.newHashSet();
+ for (String name : refs.keySet()) {
+ SnapshotRef ref = refs.get(name);
+ Long maxRefAgeMs = ref.maxRefAgeMs() != null ? ref.maxRefAgeMs() :
this.defaultMaxRefAgeMs;
Review Comment:
> defaultMaxRefAgeMs can be null if the table doesn't have a max age
defined. In that case, we shouldn't age off any refs. The max age is infinite.
I thought this originally as well, but the table configuration spec
https://github.com/apache/iceberg/blob/6f4ba696f8819a580aad3032b86838826c194c6a/docs/tables/configuration.md
states that the default for history.expire.max-ref-age-ms would be
Long.MAX_VALUE to indicate that the max ref age is "infinite".
> So the problem is actually that the instance field is a long but it could
actually be null so this should be Long. And the code below should handle the
case where max ref age is null and keep the ref.
Given the first part, I don't think it can be null (we would default to
Long.MAX_VALUE). The comparison we are making in the code to determine if a ref
should be expired: `now - snapshot.timestampMillis() >= maxRefAgeMs`, if
maxRefAgeMs is Long.MAX_VALUE that effectively means the ref won't be expired.
Or is there some overflow case I am missing (I don't think it should be
possible) and we want to have null be the default for no expiration?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
