pvary commented on code in PR #15038:
URL: https://github.com/apache/iceberg/pull/15038#discussion_r2683026661
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -296,53 +301,51 @@ private Map<String, SnapshotRef>
computeRetainedRefs(Map<String, SnapshotRef> re
return retainedRefs;
}
- private Set<Long> computeAllBranchSnapshotsToRetain(Collection<SnapshotRef>
refs) {
- Set<Long> branchSnapshotsToRetain = Sets.newHashSet();
+ private void computeAllBranchSnapshotsToRetain(
+ Collection<SnapshotRef> refs, Set<Long> idsToRetain, Set<Long>
referencedSnapshotIds) {
for (SnapshotRef ref : refs) {
if (ref.isBranch()) {
long expireSnapshotsOlderThan =
ref.maxSnapshotAgeMs() != null ? now - ref.maxSnapshotAgeMs() :
defaultExpireOlderThan;
int minSnapshotsToKeep =
ref.minSnapshotsToKeep() != null ? ref.minSnapshotsToKeep() :
defaultMinNumSnapshots;
- branchSnapshotsToRetain.addAll(
- computeBranchSnapshotsToRetain(
- ref.snapshotId(), expireSnapshotsOlderThan,
minSnapshotsToKeep));
- }
- }
-
- return branchSnapshotsToRetain;
- }
-
- private Set<Long> computeBranchSnapshotsToRetain(
- long snapshot, long expireSnapshotsOlderThan, int minSnapshotsToKeep) {
- Set<Long> idsToRetain = Sets.newHashSet();
- for (Snapshot ancestor : SnapshotUtil.ancestorsOf(snapshot,
base::snapshot)) {
- if (idsToRetain.size() < minSnapshotsToKeep
- || ancestor.timestampMillis() >= expireSnapshotsOlderThan) {
- idsToRetain.add(ancestor.snapshotId());
+ computeBranchSnapshotsToRetain(
+ ref.snapshotId(),
+ expireSnapshotsOlderThan,
+ minSnapshotsToKeep,
+ idsToRetain,
+ referencedSnapshotIds);
} else {
- return idsToRetain;
+ referencedSnapshotIds.add(ref.snapshotId());
}
}
-
- return idsToRetain;
}
- private Set<Long> unreferencedSnapshotsToRetain(Collection<SnapshotRef>
refs) {
- Set<Long> referencedSnapshots = Sets.newHashSet();
- for (SnapshotRef ref : refs) {
- if (ref.isBranch()) {
- for (Snapshot snapshot : SnapshotUtil.ancestorsOf(ref.snapshotId(),
base::snapshot)) {
- referencedSnapshots.add(snapshot.snapshotId());
+ private void computeBranchSnapshotsToRetain(
+ long snapshot,
+ long expireSnapshotsOlderThan,
+ int minSnapshotsToKeep,
+ Set<Long> idsToRetain,
+ Set<Long> referencedSnapshotIds) {
+ boolean stillRetaining = true;
+
+ for (Snapshot ancestor : SnapshotUtil.ancestorsOf(snapshot,
base::snapshot)) {
+ referencedSnapshotIds.add(ancestor.snapshotId());
+ if (stillRetaining) {
+ if (idsToRetain.size() < minSnapshotsToKeep
+ || ancestor.timestampMillis() >= expireSnapshotsOlderThan) {
+ idsToRetain.add(ancestor.snapshotId());
+ } else {
+ stillRetaining = false;
Review Comment:
Why do we don't return here as it was before?
--
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]
