You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2025/07/21 13:14:43 UTC
[PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
amogh-jahagirdar opened a new pull request, #13614:
URL: https://github.com/apache/iceberg/pull/13614
Fixes #13568
Currently in the core implementation of file cleanup as part of expires snapshots, it's possible that _all_ non-main references are removed after a snapshot expiration; the decision to select incremental cleanup or the reachable cleanup is simply based on the table metadata after expiration having a single reference. However, this is not correct since full reachability is required if the table metadata before expiration had other refs.
Additionally, the incremental cleanup would currently just get the first ref from the metadata before expiration assuming there was only one, and this can just be some non-main branch or tag, and then the ancestry calculation for incremental cleanup is incorrect.
The following changes are made in this PR:
1. Incremental cleanup is performed only if table metadata before and after ("after" being the table metadata after the commit, it can include any intermediate changes after the expiration was committed) has only one ref (the main ref)
2. Adding additional validation in Incremental cleanup and changing Iterables.getFirst in incremental cleanup to `getOnlyElement` so there's an explicit failure in case that violation somehow gets broken
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226791798
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I don't think we'd ever be in a state where only a non main ref is defined because at minimum the `main` branch would always be defined; even if it's empty. so the exactly one check is OK i think. I could make it more explicit though by comparing that it is main. `base.currentSnapshot()` would also not be null at this point since we early return on 355 if there are no snapshots in the metadata before, there'd be nothing to cleanup.
>We also need to ensure that there were no non-main snapshots before expiration. Those need to be cleaned up through reachability analysis.
The ancestry check in the method in 407 would cover this because it checks that any removed snapshots are not outside the main ancestry
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226914732
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -352,7 +352,7 @@ public void commit() {
});
LOG.info("Committed snapshot changes");
- if (cleanExpiredFiles) {
+ if (cleanExpiredFiles && base.currentSnapshot() != null) {
Review Comment:
i think I will need to fix this, it's possible that the metadata before expiration has no main but has other files to clean up as part of other branches! This is a good candidate for a test
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2236915300
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1107,21 +1158,23 @@ public void testIncrementalCleanupFailsWhenExpiringSnapshotId() {
@TestTemplate
public void testMultipleRefsAndCleanExpiredFilesFailsForIncrementalCleanup() {
+ assumeThat(incrementalCleanup).isTrue();
+
table.newAppend().appendFile(FILE_A).commit();
table.newDelete().deleteFile(FILE_A).commit();
- table.manageSnapshots().createTag("TagA", table.currentSnapshot().snapshotId()).commit();
- waitUntilAfter(table.currentSnapshot().timestampMillis());
- RemoveSnapshots removeSnapshots = (RemoveSnapshots) table.expireSnapshots();
Review Comment:
This had to change because now for this case we're being a bit more permissive. Previously any ref existiing would lead to a failure but this test was setting up a tag on main which got aged off. With the new logic we'd allow incremental cleanup here since the tag is on main and we can use incremental cleanup safely here. I updated the test so that it creates a branch and writes to the branch which is then aged off. The check that there are no snapshots being removed outside of main will fail in the new test.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223364190
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1830,32 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 50;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs + 10);
Review Comment:
Why +10? `waitUntilAfter` should handle it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this a bit so that we can distinguish where the only _branch_ is main before expiration and after expiration we validate that there's no other _refs_ other than main.
That way so long as any tags on the ancestry of main age out we can incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The non main snapshots are checked at the very end.
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227018502
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +391,41 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
+ if (specifiedSnapshotId) {
+ throw new UnsupportedOperationException(
+ "Cannot clean files incrementally when snapshot IDs are specified");
+ }
+
+ if (!hasOnlyMainBranch(base) || !hasOnlyMainBranch(current)) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files for tables with more than 1 ref");
+ }
+
+ if (!allSnapshotsAreInMain(base) || !allSnapshotsAreInMain(current)) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files when there are snapshots outside of main");
+ }
+ }
+
+ private boolean hasOnlyMainBranch(TableMetadata metadata) {
+ return metadata.refs().size() == 1 && metadata.refs().containsKey(SnapshotRef.MAIN_BRANCH);
Review Comment:
Ok, I forgot that we actually don't return a main ref with -1, no ref would be surfaced. I'll need to fix that as well
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2229991194
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1879,91 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs);
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ removeSnapshots(table)
+ .cleanExpiredFiles(true)
+ .expireOlderThan(System.currentTimeMillis())
+ .deleteWith(deletedFiles::add)
+ .commit();
+ assertThat(deletedFiles).isEqualTo(expectedDeleteFiles);
+ }
+
+ @TestTemplate
+ public void testCannotIncrementallyCleanupBranchBeforeExpiration() {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .deleteWith(deletedFiles::add)
+ .expireOlderThan(System.currentTimeMillis())
+ .commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(
+ "Cannot incrementally clean files when metadata before expiration has other branches");
+ }
+
+ @TestTemplate
+ public void testReachableCleanupWhenBranchAgedOff() {
+ assumeThat(incrementalCleanup).isFalse();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+ Snapshot latestTestSnapshot = table.snapshot(branch);
+ long currentTime = System.currentTimeMillis();
+ waitUntilAfter(currentTime + branchAgeMs);
+
+ Set<String> expectedDeletedFiles =
+ ImmutableSet.of(
+ latestTestSnapshot.manifestListLocation(),
+ Iterables.getOnlyElement(latestTestSnapshot.allManifests(table.io())).path(),
+ FILE_A.location());
Review Comment:
I've updated the algorithm but this test case will still fail for the incremental case since the incremental cleanup doesn't cleanup FILE_A when it should be able to.
Incremental should be able to apply for this case because the commit graph looks like:
Before expiration:
```
Snapshot with FILE_A --- Snapshot with FILE_A_REMOVED (main, branch)
```
After expiration:
```
Snapshot with FILE_A_REMOVED (main)
```
In the incremental case only the manifest list and manifest from the first snapshot are removed.
I think this problem is independent of the branching, even with just removing the branching from the test and just committing to main, FILE_A still doesn't end up getting cleaned up when I would expect it to be cleaned up since the new manifest would have a DELETED entry for it, and the file is orphaned.
I also confirmed that this is independent of my changes here, the issue can be reproduced in my local master branch.
I'll need to dig into this a bit more, but I think it's something missing in the incremental cleanup logic to detect the ability to cleanup FILE_A.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this a bit so that we can distinguish where there only branch is main before expiration and after expiration we validate that there's no other refs other than main. That way so long as any tags on the ancestry of main age out we can incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The non main snapshots are checked at the very end.
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "sqd (via GitHub)" <gi...@apache.org>.
sqd commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2222702883
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -375,7 +375,7 @@ private void cleanExpiredSnapshots() {
}
if (incrementalCleanup == null) {
- incrementalCleanup = current.refs().size() == 1;
+ incrementalCleanup = base.refs().size() == 1 && current.refs().size() == 1;
Review Comment:
If incrementalCleanup is explicitly enabled by `withIncrementalCleanup(true)`, but on this line we already know that it's going to crash, does it make sense to fallback to full scan with a warning, or just crash here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#issuecomment-3128448908
OK thanks for the reviews @rdblue @sqd @stevenzwu ! I'll go ahead and merge
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2236915300
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1107,21 +1158,23 @@ public void testIncrementalCleanupFailsWhenExpiringSnapshotId() {
@TestTemplate
public void testMultipleRefsAndCleanExpiredFilesFailsForIncrementalCleanup() {
+ assumeThat(incrementalCleanup).isTrue();
+
table.newAppend().appendFile(FILE_A).commit();
table.newDelete().deleteFile(FILE_A).commit();
- table.manageSnapshots().createTag("TagA", table.currentSnapshot().snapshotId()).commit();
- waitUntilAfter(table.currentSnapshot().timestampMillis());
- RemoveSnapshots removeSnapshots = (RemoveSnapshots) table.expireSnapshots();
Review Comment:
This had to change because now for this case the logic is more permissive. Previously any ref existing would lead to a failure with incremental cleanup, however with the new logic we'd allow incremental cleanup here since the tag is aged off on main. I updated the test so that it creates a branch and writes to the branch which is then aged off. The check that there are no snapshots being removed outside of main will fail in the new test.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226791798
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I don't think we'd ever be in a state where only a non main ref is defined because at minimum the `main` branch would always be defined; even if it's empty. so the exactly one check is OK i think. I could make it more explicit though. `base.currentSnapshot()` would also not be null at this point since we early return on 355
>We also need to ensure that there were no non-main snapshots before expiration. Those need to be cleaned up through reachability analysis.
The ancestry check in the method in 407 would cover this because we'd check that we can't do incremental cleanup for any snapshots that are outside of main?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this so that we distinguish the validation from before expiration and after expiration a little bit.
Incremental cleanup can only be done
1. No specified snapshots to remove
2. Before expiration there are no _branches_ other than main.
3. After expiration there are no _refs_ other than main
4. When there are no non-main unreferenced snapshots before or after expiration.
The distinction for allowing tags before expiration is beneficial since as any tags on the ancestry of main age out we can still incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The check in 4 technically could already cover what's done in 2, but 2 is a lighter weight check to verify first (also helps to have a different error message here imo) and we fall back to 4 for the staged case
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2225866011
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -365,17 +365,13 @@ ExpireSnapshots withIncrementalCleanup(boolean useIncrementalCleanup) {
private void cleanExpiredSnapshots() {
TableMetadata current = ops.refresh();
- if (specifiedSnapshotId) {
- if (incrementalCleanup != null && incrementalCleanup) {
- throw new UnsupportedOperationException(
- "Cannot clean files incrementally when snapshot IDs are specified");
- }
-
- incrementalCleanup = false;
- }
-
- if (incrementalCleanup == null) {
- incrementalCleanup = current.refs().size() == 1;
+ if (Boolean.TRUE.equals(incrementalCleanup)) {
+ validateCleanupCanBeIncremental(current);
+ } else {
+ incrementalCleanup =
Review Comment:
This doesn't properly handle the case where incremental cleanup was explicitly set to false
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2229991194
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1879,91 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs);
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ removeSnapshots(table)
+ .cleanExpiredFiles(true)
+ .expireOlderThan(System.currentTimeMillis())
+ .deleteWith(deletedFiles::add)
+ .commit();
+ assertThat(deletedFiles).isEqualTo(expectedDeleteFiles);
+ }
+
+ @TestTemplate
+ public void testCannotIncrementallyCleanupBranchBeforeExpiration() {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .deleteWith(deletedFiles::add)
+ .expireOlderThan(System.currentTimeMillis())
+ .commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(
+ "Cannot incrementally clean files when metadata before expiration has other branches");
+ }
+
+ @TestTemplate
+ public void testReachableCleanupWhenBranchAgedOff() {
+ assumeThat(incrementalCleanup).isFalse();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+ Snapshot latestTestSnapshot = table.snapshot(branch);
+ long currentTime = System.currentTimeMillis();
+ waitUntilAfter(currentTime + branchAgeMs);
+
+ Set<String> expectedDeletedFiles =
+ ImmutableSet.of(
+ latestTestSnapshot.manifestListLocation(),
+ Iterables.getOnlyElement(latestTestSnapshot.allManifests(table.io())).path(),
+ FILE_A.location());
Review Comment:
I've updated the algorithm but this test case will still fail for the incremental case since the incremental cleanup doesn't cleanup FILE_A when it should be able to.
Incremental should be able to apply for this case because the commit graph looks like:
Before expiration:
```
Snapshot with FILE_A --- Snapshot with FILE_A_REMOVED (main, branch)
```
After expiration:
```
Snapshot with FILE_A_REMOVED (main)
```
In the incremental case only the manifest list and manifest from the first snapshot are removed. Reachability cleans all 3 files as expected.
I think this problem is independent of the branching, even with just removing the branching from the test and just committing to main, FILE_A still doesn't end up getting cleaned up when I would expect it to be cleaned up since the new manifest would have a DELETED entry for it, and the file is orphaned.
I also confirmed that this is independent of my changes here, the issue can be reproduced in my local master branch.
I'll need to dig into this a bit more, but I think it's something missing in the incremental cleanup logic to detect the ability to cleanup FILE_A. cc @rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223274853
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,11 +49,6 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
- if (afterExpiration.refs().size() > 1) {
- throw new UnsupportedOperationException(
- "Cannot incrementally clean files for tables with more than 1 ref");
- }
Review Comment:
I upleveled this validation to `RemoveSnapshots`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2237507698
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -352,7 +352,7 @@ public void commit() {
});
LOG.info("Committed snapshot changes");
- if (cleanExpiredFiles) {
+ if (cleanExpiredFiles && !base.snapshots().isEmpty()) {
Review Comment:
Yeah it does, I just updated it this way so that the logic short circuits before that point.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2219196348
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,7 +50,7 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
Review Comment:
I think there's a bigger question of moving away from incremental cleanup and just always doing the reachability analysis in the core cleanup. This is what's done in spark and other integrations (which is probably why this issue didn't surfaced for quite some time).
If I recall correctly, the original intent for keeping incremental cleanup was that at the time branching/tagging was a pretty new introduction and I didn't want to regress folks expiration cleanup in the "average" case. However, even beyond branching/tagging there are so many constructs on table metadata that can make incremental cleanup difficult to do safely, that maybe in 2.0 we should consider just removing it and always doing a reachability analysis? cc @rdblue @RussellSpitzer @stevenzwu @aokolnychyi
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "sqd (via GitHub)" <gi...@apache.org>.
sqd commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2222741581
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,7 +50,7 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
- if (afterExpiration.refs().size() > 1) {
+ if (beforeExpiration.refs().size() > 1 || afterExpiration.refs().size() > 1) {
Review Comment:
> we could check that all refs are in the main branch's history and allow it
I agree. It's the safest to only allow incremental cleanup when this condition is met. This should also makes incremental cleanup safe for a large class of history manipulations that's going to get implemented in the future.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223364763
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1830,32 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 50;
Review Comment:
Could this be 20 instead? These things add up.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223360345
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -236,7 +236,7 @@ public void testExpireOlderThanWithRollback() {
Set<String> deletedFiles = Sets.newHashSet();
- removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
+ table.expireSnapshots().expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
Review Comment:
Should we copy and validate the exception thrown for these cases, rather than exclude them from tests?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226791798
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I don't think we'd ever be in a state where only a non main ref is defined because at minimum the `main` branch would always be defined; even if it's empty. so the exactly one check is OK i think. I could make it more explicit though
>We also need to ensure that there were no non-main snapshots before expiration. Those need to be cleaned up through reachability analysis.
The ancestry check in the method in 407 would cover this because we'd check that we can't do incremental cleanup for any snapshots that are outside of main?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this so that we distinguish the validation from before expiration and after expiration a little bit.
Incremental cleanup can only be done
1. No specified snapshots to remove
2. Before expiration there are no _branches_ other than main.
3. After expiration there are no _refs_ other than main
4. When there are no non-main unreferenced snapshots before or after expiration.
The distinction for allowing tags before expiration is beneficial since as any tags on the ancestry of main age out we can still incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The check in 4 technically could already cover what's done in 2, but 2 is a lighter weight check to verify first and we fall back to 4 for the staged case
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2229043800
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I think maybe we can do a little better. I think your argument for tags main's ancestors is a good one. We can do that for branches, too.
What about these rules to determine if incremental is safe?
1. No specified snapshots to remove
2. No snapshots to remove that are not ancestors of main
3. No snapshots after expiration that are not ancestors of main
I think the combination of 2 and 3 cover branch and tag cases. Tags function like branches with just one snapshot, so reasoning about branches will cover them. Any branch either has a snapshot outside of main or it doesn't. If it doesn't, then incremental cleanup will work. If there is a snapshot outside of main, then it is either going to be caught be 2 (if it ages off) or 3 (if it does not age off).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226914732
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -352,7 +352,7 @@ public void commit() {
});
LOG.info("Committed snapshot changes");
- if (cleanExpiredFiles) {
+ if (cleanExpiredFiles && base.currentSnapshot() != null) {
Review Comment:
i think I will need to fix this, it's possible that the metadata before expiration has no main but has other files to clean up!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227358908
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1879,91 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs);
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ removeSnapshots(table)
+ .cleanExpiredFiles(true)
+ .expireOlderThan(System.currentTimeMillis())
+ .deleteWith(deletedFiles::add)
+ .commit();
+ assertThat(deletedFiles).isEqualTo(expectedDeleteFiles);
+ }
+
+ @TestTemplate
+ public void testCannotIncrementallyCleanupBranchBeforeExpiration() {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .deleteWith(deletedFiles::add)
+ .expireOlderThan(System.currentTimeMillis())
+ .commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(
+ "Cannot incrementally clean files when metadata before expiration has other branches");
+ }
+
+ @TestTemplate
+ public void testReachableCleanupWhenBranchAgedOff() {
+ assumeThat(incrementalCleanup).isFalse();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+ Snapshot latestTestSnapshot = table.snapshot(branch);
+ long currentTime = System.currentTimeMillis();
+ waitUntilAfter(currentTime + branchAgeMs);
+
+ Set<String> expectedDeletedFiles =
+ ImmutableSet.of(
+ latestTestSnapshot.manifestListLocation(),
+ Iterables.getOnlyElement(latestTestSnapshot.allManifests(table.io())).path(),
+ FILE_A.location());
Review Comment:
Previously, we'd select incremental and miss out on cleaning up FILE_A because the check was only limited to currently active refs. Now, we'll use the reachable analysis to determine that FILE_A can be cleaned up since the branch aged off and on the main branch it's deleted.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226791798
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I don't think we'd ever be in a state where only a non main ref is defined because at minimum the `main` branch would always be defined; even if it's empty. so the exactly one check is OK i think. I could make it more explicit though by comparing that it is main. `base.currentSnapshot()` would also not be null at this point since we early return on 355 if there are no snapshots in the metadata before, there'd be nothing to cleanup.
>We also need to ensure that there were no non-main snapshots before expiration. Those need to be cleaned up through reachability analysis.
The ancestry check in the method in 407 would cover this because we'd check that we can't do incremental cleanup for any snapshots that are outside of main?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2231905291
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
After a lot of sketching out to prove to myself this works, I agree with the simplified algorithm. I went ahead and updated the implementation
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226802628
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
+ if (specifiedSnapshotId) {
+ throw new UnsupportedOperationException(
+ "Cannot clean files incrementally when snapshot IDs are specified");
+ }
+
+ if (current.refs().size() > 1) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files for tables with more than 1 ref");
+ }
+
+ if (!allRemovedSnapshotsAreInMainAncestry(current)) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files when snapshots outside of main ancestry have been removed");
+ }
+ }
+
+ private boolean allRemovedSnapshotsAreInMainAncestry(TableMetadata current) {
+ Set<Long> snapshotsBeforeExpiration =
+ base.snapshots().stream().map(Snapshot::snapshotId).collect(Collectors.toSet());
Review Comment:
Don't need the extra set here, can just iterate over it
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226814598
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
Ah but even if a snapshot is not removed we still would need reachable analysis if there's a non-main snapshot since a staged but not removed snapshot could still reference a file that should still not be cleaned up
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2236930843
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1883,67 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
Review Comment:
move `expectedDeleteFiles` closer to where it's actually used
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2220580612
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,7 +50,7 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
- if (afterExpiration.refs().size() > 1) {
+ if (beforeExpiration.refs().size() > 1 || afterExpiration.refs().size() > 1) {
Review Comment:
Could this leak files if there is a rollback or a staged (but not referenced) commit?
For instance, if you have snapshots like this:
```
A --- B --- C --- D (main)
`--- X (unreferenced)
```
If snapshot X should be cleaned up, Iceberg should not use the incremental strategy. Also, if X is not removed but B and C are, then incremental also can't be used because C may have removed files from X.
Instead of checking the number of refs, I think we need to check the history of the main branch. If there is any snapshot outside of the main branch before or after expiration, incremental cannot be used. There are cases where a ref is in the main branch's history that are safe, but I would probably keep the implementation simpler and avoid checking for things like branch overlap.
What I would do is:
1. If there is more than one ref after expiration, use the reference set strategy (optionally, we could check that all refs are in the main branch's history and allow it)
2. If there are any snapshots to be removed after expiration that were not the main branch's ancestors, use the reference set strategy
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223247804
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -81,12 +75,11 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
return;
}
- SnapshotRef branchToCleanup = Iterables.getOnlyElement(beforeExpiration.refs().values(), null);
- if (branchToCleanup == null) {
+ Snapshot latest = beforeExpiration.currentSnapshot();
+ if (latest == null) {
Review Comment:
I think I had originally implemented this so that incremental cleanup can be performed on singular non-main branches (i.e. main is empty and there's some `test` branch which got cleaned up) but I do think that's a pretty narrow case to optimize for.
I think going back to just always taking the latest on main on the metadata before expiration and deriving ancestry from that for incremental cleanup is safest and simplest.
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -236,7 +236,7 @@ public void testExpireOlderThanWithRollback() {
Set<String> deletedFiles = Sets.newHashSet();
- removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
+ table.expireSnapshots().expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
Review Comment:
@rdblue @sqd We actually already have some test cases around the whole rollback/staged case, and with the new check for main ancestry we'd end up failing since `removeSnapshots(table)` parameterizes around both incremental and not incremental.
I generally agree around biasing to the reachable analysis even if it technically could be incremental since reachability is always a clearly correct way of cleaning files but just want to call out that users can end up failing if they end up explicitly specifying incremental and also wanted to call out why I modified these existing tests.
I also don't think this is a huge deal since the whole `withIncremental` API is not on the interface but requires a user to cast to `RemoveSnapshots` implementation (we added this API initially for testing against both cases and making sure the new reachable analysis was also correct)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2224259963
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -256,8 +257,40 @@ public void testExpireOlderThanWithRollback() {
);
}
+ @TestTemplate
+ public void testCannotCleanupIncrementallyWithRollback() {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+
+ Snapshot firstSnapshot = table.currentSnapshot();
+ assertThat(firstSnapshot.allManifests(table.io())).hasSize(1);
+
+ waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ table.newDelete().deleteFile(FILE_B).commit();
+
+ Snapshot secondSnapshot = table.currentSnapshot();
+
+ table.manageSnapshots().rollbackTo(firstSnapshot.snapshotId()).commit();
+
+ long tAfterCommits = waitUntilAfter(secondSnapshot.timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .expireOlderThan(tAfterCommits)
+ .deleteWith(deletedFiles::add)
+ .commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(
+ "Cannot incrementally clean files when snapshots outside of main ancestry have been removed");
+ }
+
@TestTemplate
public void testExpireOlderThanWithRollbackAndMergedManifests() {
+ assumeThat(incrementalCleanup).isFalse();
Review Comment:
I didn't copy this case again for the incremental failure case, since this case is no different than the case above beyond merging manifests or not; the current approach blanket prevents performing incremental cleanup in case of rollbacks so both cases just fail the same.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2224262720
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1830,32 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 50;
Review Comment:
Done!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this a bit so that we can distinguish where there only _branch_ is main before expiration and after expiration we validate that there's no other refs other than main. That way so long as any tags on the ancestry of main age out we can incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The non main snapshots are checked at the very end.
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2237411675
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -352,7 +352,7 @@ public void commit() {
});
LOG.info("Committed snapshot changes");
- if (cleanExpiredFiles) {
+ if (cleanExpiredFiles && !base.snapshots().isEmpty()) {
Review Comment:
This seems odd. Shouldn't this happen automatically when `cleanExpiredSnapshots()` gathers the set of snapshots to clean?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#issuecomment-3128423935
This look good to me. Thanks, @amogh-jahagirdar!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2219196348
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,7 +50,7 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
Review Comment:
I think there's a bigger question of moving away from incremental cleanup and just always doing the reachability analysis in the core cleanup. This is what's done in spark and other integrations (which is probably why this issue didn't surfaced for quite some time).
If I recall correctly, the original intent for keeping incremental cleanup was that at the time branching/tagging was a pretty new introduction and I didn't want to regress folks expiration cleanup in the "average" case. However, even beyond branching/tagging there are other constructs on table metadata (like the ability to cherry pick) that can make incremental cleanup difficult to do safely, that maybe in 2.0 we should consider just removing it and always doing a reachability analysis? cc @rdblue @RussellSpitzer @stevenzwu @aokolnychyi
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this a bit so that we can distinguish where there only _branch_ is main before expiration and after expiration we validate that there's no other _refs_ other than main.
That way so long as any tags on the ancestry of main age out we can incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The non main snapshots are checked at the very end.
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227263522
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +391,60 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
+ if (specifiedSnapshotId) {
+ throw new UnsupportedOperationException(
+ "Cannot clean files incrementally when snapshot IDs are specified");
+ }
+
+ if (!onlyRefIsMain(current)) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files for tables with more than 1 ref");
+ }
+
+ if (!onlyBranchIsMain(base)) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files when metadata before expiration has other branches");
+ }
Review Comment:
Need a test case for this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2222832174
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,7 +50,7 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
- if (afterExpiration.refs().size() > 1) {
+ if (beforeExpiration.refs().size() > 1 || afterExpiration.refs().size() > 1) {
Review Comment:
I think in the current implementation we're safe against staged snapshots being cleaned up since the incremental cleanup itself will delete if the manifests are not referenced in an ancestor.
That said, I generally agree that the clearest way to determine if a reachability analysis should be performed is by doing an analysis of if any of the snapshots being removed are outside of the main ancestry.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2224263964
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1830,32 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 50;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs + 10);
Review Comment:
It's not needed, I cleaned this up
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223240990
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -236,7 +236,7 @@ public void testExpireOlderThanWithRollback() {
Set<String> deletedFiles = Sets.newHashSet();
- removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
+ table.expireSnapshots().expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
Review Comment:
@rdblue @sqd We actually already have some test cases around the whole rollback/staged cases, and with the new check for main ancestry we'd end up failing since `removeSnapshots(table)` parameterizes around both incremental and not incremental.
I generally agree around biasing to the reachable analysis even if it technically could be incremental since reachability is always a clearly correct way of cleaning files but just want to call out that users can end up failing if they end up explicitly specifying incremental and also wanted to call out why I modified these existing tests.
I also don't think this is a huge deal since the whole `withIncremental` API is not on the interface but requires a user to cast to `RemoveSnapshots` implementation (we added this API initially for testing against both cases and making sure the new reachable analysis was also correct)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2229991194
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1879,91 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs);
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ removeSnapshots(table)
+ .cleanExpiredFiles(true)
+ .expireOlderThan(System.currentTimeMillis())
+ .deleteWith(deletedFiles::add)
+ .commit();
+ assertThat(deletedFiles).isEqualTo(expectedDeleteFiles);
+ }
+
+ @TestTemplate
+ public void testCannotIncrementallyCleanupBranchBeforeExpiration() {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .deleteWith(deletedFiles::add)
+ .expireOlderThan(System.currentTimeMillis())
+ .commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(
+ "Cannot incrementally clean files when metadata before expiration has other branches");
+ }
+
+ @TestTemplate
+ public void testReachableCleanupWhenBranchAgedOff() {
+ assumeThat(incrementalCleanup).isFalse();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+ Snapshot latestTestSnapshot = table.snapshot(branch);
+ long currentTime = System.currentTimeMillis();
+ waitUntilAfter(currentTime + branchAgeMs);
+
+ Set<String> expectedDeletedFiles =
+ ImmutableSet.of(
+ latestTestSnapshot.manifestListLocation(),
+ Iterables.getOnlyElement(latestTestSnapshot.allManifests(table.io())).path(),
+ FILE_A.location());
Review Comment:
I've updated the algorithm but this test case will still fail for the incremental case since the incremental cleanup doesn't cleanup FILE_A when it should be able to.
Incremental should be able to apply for this case because the commit graph looks like:
Before expiration:
```
Snapshot with FILE_A --- Snapshot with FILE_A_REMOVED (main, branch)
```
After expiration:
```
Snapshot with FILE_A_REMOVED (main)
```
In the incremental case only the manifest list and manifest from the first snapshot are removed. Reachability cleans all 3 files as expected.
I think this problem is independent of the branching, even with just removing the branching from the test and just committing to main, FILE_A still doesn't end up getting cleaned up when I would expect it to be cleaned up since the new manifest would have a DELETED entry for it, and the file is orphaned.
I also confirmed that this is independent of my changes here, the issue can be reproduced in my local master branch.
I'll need to dig into this a bit more, but I think it's something missing in the incremental cleanup logic to detect the ability to cleanup FILE_A.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2231902383
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1829,6 +1879,91 @@ public void testRemoveSnapshotsNoOp() throws Exception {
.isSameAs(current);
}
+ @TestTemplate
+ public void testFileCleanupOnAllRefsAgedOff() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Set<String> expectedDeleteFiles =
+ ImmutableSet.of(table.currentSnapshot().manifestListLocation());
+ String tag = "tag";
+ long tagAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createTag(tag, table.currentSnapshot().snapshotId())
+ .setMaxRefAgeMs(tag, tagAgeMs)
+ .commit();
+ long currentTime = System.currentTimeMillis();
+ table.newAppend().appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ waitUntilAfter(currentTime + tagAgeMs);
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ removeSnapshots(table)
+ .cleanExpiredFiles(true)
+ .expireOlderThan(System.currentTimeMillis())
+ .deleteWith(deletedFiles::add)
+ .commit();
+ assertThat(deletedFiles).isEqualTo(expectedDeleteFiles);
+ }
+
+ @TestTemplate
+ public void testCannotIncrementallyCleanupBranchBeforeExpiration() {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+
+ Set<String> deletedFiles = Sets.newHashSet();
+ assertThatThrownBy(
+ () ->
+ removeSnapshots(table)
+ .deleteWith(deletedFiles::add)
+ .expireOlderThan(System.currentTimeMillis())
+ .commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(
+ "Cannot incrementally clean files when metadata before expiration has other branches");
+ }
+
+ @TestTemplate
+ public void testReachableCleanupWhenBranchAgedOff() {
+ assumeThat(incrementalCleanup).isFalse();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ String branch = "test";
+ long branchAgeMs = 20;
+ table
+ .manageSnapshots()
+ .createBranch(branch)
+ .setMaxRefAgeMs(branch, branchAgeMs)
+ .setMinSnapshotsToKeep(branch, 1)
+ .commit();
+ table.newDelete().deleteFile(FILE_A).commit();
+ Snapshot latestTestSnapshot = table.snapshot(branch);
+ long currentTime = System.currentTimeMillis();
+ waitUntilAfter(currentTime + branchAgeMs);
+
+ Set<String> expectedDeletedFiles =
+ ImmutableSet.of(
+ latestTestSnapshot.manifestListLocation(),
+ Iterables.getOnlyElement(latestTestSnapshot.allManifests(table.io())).path(),
+ FILE_A.location());
Review Comment:
I talked this through with @rdblue earlier today, the TLDR is that incremental cleanup actually cannot safely detect that FILE_A really can be cleaned up until the snapshot which marked it as deleted is itself removed (which is not the case). So I think we can treat this as expected behavior. I've updated this test to add another commit on main and then expiring the snapshot which would add and remove FILE_A. This verifies that FILE_A can safely be cleaned up incrementally even though there used to be a branch before because that branch was entirely on main.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #13614:
URL: https://github.com/apache/iceberg/pull/13614
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this so that we distinguish the validation from before expiration and after expiration a little bit.
Incremental cleanup can only be done
1. No specified snapshots to remove
2. Before expiration there are no _branches_ other than main.
3. After expiration there are no _refs_ other than main
4. When there are no non-main unreferenced snapshots before or after expiration.
The distinction for allowing tags before expiration is beneficial since as any tags on the ancestry of main age out we can still incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true.
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2227260971
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
I've amended this so that we distinguish the validation from before expiration and after expiration a little bit.
Incremental cleanup can only be done
1. No specified snapshots to remove
2. Before expiration there are no _branches_ other than main.
3. After expiration there are no _refs_ other than main
4. When there are no non-main unreferenced snapshots before or after expiration.
That way so long as any tags on the ancestry of main age out we can incrementally cleanup, assuming the other conditions of no other non-main snapshots holds true. The non main snapshots are checked at the very end.
For the check where there are non main snapshots it handles the case where the current snapshot is null by simply checking if the whole `snapshots` list is not empty. If it's not empty then this means there are non-main snapshots.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2222832174
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -50,7 +50,7 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
@Override
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
- if (afterExpiration.refs().size() > 1) {
+ if (beforeExpiration.refs().size() > 1 || afterExpiration.refs().size() > 1) {
Review Comment:
I think in the current implementation we're safe against staged snapshots being cleaned up since the incremental cleanup itself will only delete if it's not referenced in an ancestor. That said, I generally agree that the clearest way to determine if a reachability analysis should be performed is by doing an analysis of if any of the snapshots being removed are outside of the main ancestry.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223256780
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -375,7 +375,7 @@ private void cleanExpiredSnapshots() {
}
if (incrementalCleanup == null) {
- incrementalCleanup = current.refs().size() == 1;
+ incrementalCleanup = base.refs().size() == 1 && current.refs().size() == 1;
Review Comment:
I've upleveled some of the validation, but I do think if it's specified through the API option and it's incompatible with the state of the table we should just fail instead of falling back.
I do think that we should probably revisit removing the whole `withIncremental` option (it's not on the API but there's complication to having to be defensive against bad input where most users don't even specify it). It was added for test comparisons a long time ago but I think we know that reachability is the safest and we should probably just let the implementation hide the complexity of cleanup selection.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226755925
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
Don't we also need to check that the only table ref is `main`? This checks that there's one ref and only snapshots behind `currentSnapshot` are expired. But if `main` doesn't exist, then `allRemovedSnapshotsAreInMainAncestry` is going to fail with an NPE.
We also need to ensure that there were no non-main snapshots before expiration. Those need to be cleaned up through reachability analysis.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226906496
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
Ok I've expanded this check to be more conservative but it's guaranteed to always be correct I think:
Incremental cleanup can only be done:
1. When there's no specified snapshots to remove
2. When there's no non main refs before OR after expiration (it's a bit overly conservative here because I could have an aged off tag on the main lineage which could be cleaned up incrementally. Could probably optimize this by complicating the hasOnlyMainBranch a bit by checking the ref types etc)
3. When there are no non main snapshots before or after 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix incorrect selection of incremental cleanup in expire snapshots [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13614:
URL: https://github.com/apache/iceberg/pull/13614#discussion_r2226906496
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -390,4 +386,43 @@ private void cleanExpiredSnapshots() {
cleanupStrategy.cleanFiles(base, current);
}
+
+ private void validateCleanupCanBeIncremental(TableMetadata current) {
Review Comment:
Ok I've expanded this check to be more conservative but it's guaranteed to always be correct I think:
Incremental cleanup can only be done:
1. When there's no specified snapshots to remove
2. When there's no non main refs before OR after (it's a bit overly conservative here because I could have an aged off tag on the main lineage which could be cleaned up incrementally. Could probably optimize this by complicating the hasOnlyMainBranch a bit by checking the ref types etc)
3. When there are no non main snapshots before or after
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org