rdblue commented on code in PR #4578:
URL: https://github.com/apache/iceberg/pull/4578#discussion_r857186962
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -163,21 +180,86 @@ 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> refsToRemove = Sets.newHashSet();
+ for (String name : base.refs().keySet()) {
+ SnapshotRef ref = base.ref(name);
+ if (!name.equals(SnapshotRef.MAIN_BRANCH) && ref.maxRefAgeMs() != null) {
+ Snapshot snapshot = base.snapshot(ref.snapshotId());
+ if (removalTimeReference - snapshot.timestampMillis() >=
ref.maxRefAgeMs()) {
+ refsToRemove.add(name);
+ }
+
+ }
+
+ if (!refsToRemove.contains(name) &&
!idsToRemove.contains(ref.snapshotId())) {
Review Comment:
Should `idsToRemove` be allowed to contain snapshots from refs? I think you
should be able to remove a snapshot that is a ref and have that drop the ref. I
think that should be an error when you expire the snapshot by ID.
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -163,21 +180,86 @@ 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> refsToRemove = Sets.newHashSet();
+ for (String name : base.refs().keySet()) {
+ SnapshotRef ref = base.ref(name);
+ if (!name.equals(SnapshotRef.MAIN_BRANCH) && ref.maxRefAgeMs() != null) {
+ Snapshot snapshot = base.snapshot(ref.snapshotId());
+ if (removalTimeReference - snapshot.timestampMillis() >=
ref.maxRefAgeMs()) {
+ refsToRemove.add(name);
+ }
+
+ }
+
+ if (!refsToRemove.contains(name) &&
!idsToRemove.contains(ref.snapshotId())) {
+ idsToRetain.add(ref.snapshotId());
+ }
+
+ }
+ List<Map.Entry<String, SnapshotRef>> branches = base
Review Comment:
Don't forget style.
--
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]
