You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "hantangwangd (via GitHub)" <gi...@apache.org> on 2024/08/21 06:57:31 UTC
[PR] Core: Fix the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
hantangwangd opened a new pull request, #10983:
URL: https://github.com/apache/iceberg/pull/10983
This PR fix the behavior of `IncrementalFileCleanup`, which will lead in useful data files being mistakenly cleaned up when expire a specified snapshot.
Fix issue: #10982
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2316382335
@amogh-jahagirdar, @hantangwangd, I'm not sure that incremental cleanup is doing anything wrong here. Incremental cleanup deletes data files when the snapshot that removed them from the table is expired. If you want something safer, then the reachable strategy should be used.
I think there's a fair argument that you should not be able to use the API to so easily get in this state, but rather than changing how incremental cleanup works, I would recommend making it so the API doesn't allow expiring a specific snapshot by ID.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1739152717
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -321,6 +323,15 @@ ExpireSnapshots withIncrementalCleanup(boolean useIncrementalCleanup) {
private void cleanExpiredSnapshots() {
TableMetadata current = ops.refresh();
+ if (specifiedSnapshotId) {
+ if (incrementalCleanup != null && incrementalCleanup) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files when expiring snapshots through specified ids");
Review Comment:
"Cannot clean files incrementally when snapshot IDs are specified"?
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -370,7 +370,7 @@ public void testRetainLastWithExpireById() {
}
// Retain last 3 snapshots, but explicitly remove the first snapshot
- removeSnapshots(table).expireSnapshotId(firstSnapshotId).retainLast(3).commit();
+ table.expireSnapshots().expireSnapshotId(firstSnapshotId).retainLast(3).commit();
Review Comment:
So these are cases where we used to be able to do incremental cleanup before but now will fail. I'm good with that since the default now is more safe even though from an API perspective it may be less performant for specific cases. And to address the performant issue, practically at larger scales there will be some distributed procedure running and the Spark procedure does the full reachability analysis itself anyways.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1733319305
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
Ah good point, yes the inheritance chain isn't guaranteed since the parent also could've been expired.
OK I'll need to think about this a bit more deeply then. On the surface just seems like if someone is expiring a specific snapshot where there's a deleted entry we may want to do the reachability analysis; maybe with the file sequence number we can do a more intelligent incremental cleanup for V2 tables.
Or potentially since we have the invariant here that there's only one ref at this point, we can just validate that there are no ancestors at all.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1733265734
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
> Snapshot IDs are guaranteed to be unique but not necessarily monotonically increasing (in fact by default in the reference implementation it's random.
Thanks for your reminder, sorry for misleading by `TestTableOperations.newSnapshotId()`. Comparing the `snapshotId` is definitely wrong and I will fix it.
> I think you'll just need to check that there's no parent snapshot for each of the snapshots in the valid snapshots map.
Checking the parent snapshot may also encounter some problems, for example, we may have already expired some specified snapshots including the parent of this snapshot before. In this case, the inheritance chain between snapshots has been broken. So it seems that we shouldn't rely on it as it may lead in wrong result.
For example, if we have a table with actions as follows:
```
table.newAppend().appendFile(FILE_A).commit; // snapshotID1
table.newAppend().appendFile(FILE_B).commit; // snapshotID2
table.newDelete().deleteFile(FILE_A).commit; // snapshotID3
table.newAppend().appendFile(FILE_A2).commit; // snapshotID4
```
And we have already expired specified snapshot `snapshotID2` before.
Now when we trying to expire snapshot `snapshotID3` using the policy of `IncrementalFileCleanup`, `FILE_A` would be wrongly cleaned up if we rely on checking the parent snapshot.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2316827563
> The incremental cleanup logic is already quite complex and I'm now thinking it's not really worth it to add handling this particular case. There's probably more cases and then if we were to keep adding handling for that at some point the implementation will look like the reachable cleanup anyways.
Yes, it is indeed the case, there's probably more cases. Considering the factors you mentioned, I agree with you that maybe it's better to accept that incremental cleanup has limitations and from an API perspective ensure that callers don't fall into a state where they cleanup files that are still referenced (as @rdblue suggested).
> would you be open to changing the approach to just ensure that if expireSnapshotId is called and incrementalCleanup is set to true we fail before triggering the file cleanup? Also if incremental cleanup isn't specified I think incrementalCleanup should be set to false in the case a specific snapshotID is specified in expireSnapshotId. That achieves the safety guarantees I think we want.
Sure, I'd like to make this change, and modify the test cases accordingly. Thanks for all your explanations and suggestions @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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1737646125
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -116,6 +117,7 @@ public ExpireSnapshots cleanExpiredFiles(boolean clean) {
public ExpireSnapshots expireSnapshotId(long expireSnapshotId) {
LOG.info("Expiring snapshot with id: {}", expireSnapshotId);
idsToRemove.add(expireSnapshotId);
+ specifySnapshotIds = true;
Review Comment:
Thanks for the suggestion, 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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1733280303
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
+ }
+
+ // The file in DELETE entry can be deleted if no snapshot exists between
+ // the one that added it and this one
+ long fileSequenceNumber = entry.fileSequenceNumber() == null ? 0L : entry.fileSequenceNumber();
+ long fileDeletedSnapshotSequenceNumber =
+ expiredSnapshotIdToSequenceNumberMap.get(entry.snapshotId());
+ return fileSequenceNumber > 0
Review Comment:
> Since we're using sequence numbers this means that we're also saying for v1 tables we can't determine to clean it up in the incremental strategy?
Yes, if we have to rely on sequence numbers to determine the files to be cleaned, then we can't do it for v1 tables. It's not perfect but it is correct. And very agree with you that, it worth thinking a way to cover v1 tables.
> I agree with this statement but doesn't this just mean we need to check the parent snapshots manifest entries for this particular file to see if it was added specifically in the parent snapshot?
The same problems as described above, the inheritance chain between snapshots may have been broken before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1731788916
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -61,17 +63,21 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
// 4. Delete the manifest lists
Set<Long> validIds = Sets.newHashSet();
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap = Maps.newConcurrentMap();
for (Snapshot snapshot : afterExpiration.snapshots()) {
validIds.add(snapshot.snapshotId());
+ validSnapshotIdToSequenceNumberMap.put(snapshot.snapshotId(), snapshot.sequenceNumber());
}
Set<Long> expiredIds = Sets.newHashSet();
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap = Maps.newConcurrentMap();
Review Comment:
Same as above, `expiredSnapshotToSequenceNumber`
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
I think you'll just need to check that there's no parent snapshot for each of the snapshots in the valid snapshots map
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
+ }
+
+ // The file in DELETE entry can be deleted if no snapshot exists between
+ // the one that added it and this one
+ long fileSequenceNumber = entry.fileSequenceNumber() == null ? 0L : entry.fileSequenceNumber();
+ long fileDeletedSnapshotSequenceNumber =
+ expiredSnapshotIdToSequenceNumberMap.get(entry.snapshotId());
+ return fileSequenceNumber > 0
Review Comment:
Actually we may not need to rely on the sequence numbers:
```
// The file in DELETE entry can be deleted if no snapshot exists between
// the one that added it and this one
```
I agree with this statement but doesn't this just mean we need to check the parent snapshots manifest entries for this particular file to see if it was added specifically in the parent snapshot? Although this is another manifest read I feel like it's OK (it's not as intensive as the reachable cleanup case/still feels incremental enough) and I think it should also solve the V1 case.
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -61,17 +63,21 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
// 4. Delete the manifest lists
Set<Long> validIds = Sets.newHashSet();
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap = Maps.newConcurrentMap();
Review Comment:
`validSnapshotToSequenceNumber` or `retainedSnapshotToSequenceNumber`? The map suffix is redundant.
Also with this new approach, we may also need to look at refactoring to get rid of the other `validIds` set since now we can just use this new map
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
+ }
+
+ // The file in DELETE entry can be deleted if no snapshot exists between
+ // the one that added it and this one
+ long fileSequenceNumber = entry.fileSequenceNumber() == null ? 0L : entry.fileSequenceNumber();
+ long fileDeletedSnapshotSequenceNumber =
+ expiredSnapshotIdToSequenceNumberMap.get(entry.snapshotId());
+ return fileSequenceNumber > 0
Review Comment:
Since we're using sequence numbers this means that we're also saying for v1 tables we can't determine to clean it up in the incremental strategy? I think that's OK since orphan files can clean it up later. Worth thinking through other ways to determine this beyond leveraging sequence numbers.
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
This is not right since we cannot use the snapshot ID as an ordering. Snapshot IDs are guaranteed to be unique but not necessarily monotonically increasing (in fact by default in the reference implementation it's random https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/SnapshotIdGeneratorUtil.java#L36)
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2314425466
So when I wrote the Spark procedure for this we were already aware that this code path has a lot of potential issues. We end up basically completely rewriting the logic of detecting what is used and what is not used here which has made it a bit difficult to keep up to date. I wonder if now is the time to just re-implement it to work more like the Spark Action and just take the diff between reachable before and after.
That said I'll try to take a look at see if the fixes make sense.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1733319305
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
Ah good point, yes the inheritance chain isn't guaranteed since the parent also could've been expired.
OK I'll need to think about this a bit more deeply then. On the surface just seems like if someone is expiring a specific snapshot where there's a deleted entry we may want to do the 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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #10983:
URL: https://github.com/apache/iceberg/pull/10983
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1733319305
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
Ah good point, yes the inheritance chain isn't guaranteed since the parent also could've been expired.
OK I'll need to think about this a bit more deeply then. On the surface just seems like if someone is expiring a specific snapshot where there's a deleted entry we may want to do the reachability analysis; maybe with the file sequence number we can do a more intelligent incremental cleanup for V2 tables.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2316763362
@hantangwangd I thought about this a bit more and while I still think updating the IncrementalFileCleanup to address this particular case is possible, I think the question more so is "Should we?".
The main problem we want to address is that the ExpireSnapshots implementation should ensure that a user doesn't end up in a state where they cleanup files that are still referenced. Looking at what's going on in Presto we're actually casting to `RemoveSnapshots` implementation and the implementation exposes `withIncrementalCleanup`.
The reason for exposing that in the implementation (and not the interface) was when we were doing branching/tagging we wanted to verify in tests that both cleanup strategies work as expected so it was easy to parameterize around that API. Ideally we wouldn't need to expose such an API and in the implementation intelligently choose the right strategy, but the validation from tests is really important.
So considering `withIncrementalCleanup` is exposed to someone who casts to RemoveSnapshots then we can either choose to complicate the Incremental cleanup with more logic to handle this specific case or we can accept that incremental cleanup has these limitations and from an API perspective ensure that callers don't shoot themselves in the foot and removing files which shouldn't be removed by triggering incremental cleanup when there's an arbitrary snapshot that's removed (what @rdblue suggested).
The incremental cleanup logic is already quite complex and I'm now thinking it's not really worth it to add handling this particular case. There's probably more cases and then if we were to keep adding handling for that at some point the implementation will look like the reachable cleanup anyways.
@hantangwangd would you be open to changing the approach to just ensure that if `expireSnapshotId` is called and incrementalCleanup is set to true we fail before triggering the file cleanup? https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L323
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1739706862
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -370,7 +370,7 @@ public void testRetainLastWithExpireById() {
}
// Retain last 3 snapshots, but explicitly remove the first snapshot
- removeSnapshots(table).expireSnapshotId(firstSnapshotId).retainLast(3).commit();
+ table.expireSnapshots().expireSnapshotId(firstSnapshotId).retainLast(3).commit();
Review Comment:
Thank you very much for this explanation. Yes, this change chooses a more safer strategy when expiring snapshots through specifying snapshot IDs, so users will not fall into the dangerous state described above.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1731784757
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
I think you'll just need to check that there's no parent snapshot for each of the snapshots in the valid snapshots map. In case of multiple refs, we already will fail to do incremental cleanup earlier and clients have to go through the reachable cleanup path.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2317476054
Following the discussions above, I changed the approach to just ensure that users will not fall into a state where they cleanup files that are still referenced:
- If `expireSnapshotId` is called and `incrementalCleanup` is set explicitly to `true`, we fail before triggering the file cleanup.
- If `expireSnapshotId` is called and `incrementalCleanup` isn't specified, we set `incrementalCleanup` to `false` so that reachable cleanup will be used.
Please take a look when convenient, thanks a lot! @rdblue @amogh-jahagirdar @RussellSpitzer
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2316825435
Thank you very much for your guidance @rdblue, it makes sense to avoid users falling into such a state in advance.
--
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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1737259547
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -321,6 +323,14 @@ ExpireSnapshots withIncrementalCleanup(boolean useIncrementalCleanup) {
private void cleanExpiredSnapshots() {
TableMetadata current = ops.refresh();
+ if (specifySnapshotIds) {
+ if (incrementalCleanup != null && incrementalCleanup) {
+ throw new UnsupportedOperationException(
+ "Cannot incrementally clean files when expiring snapshots through specified ids");
+ }
+ incrementalCleanup = false;
Review Comment:
Style nit: Newline after the if block
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1171,6 +1172,27 @@ public void testBranchExpiration() {
assertThat(table.ops().current().ref(SnapshotRef.MAIN_BRANCH)).isNotNull();
}
+ @TestTemplate
+ public void testSpecifySnapshotIdsAndCleanExpiredFilesFailsForIncrementalCleanup() {
Review Comment:
`testIncrementalCleanupFailsWhenExpiringSnapshotId`?
##########
core/src/main/java/org/apache/iceberg/RemoveSnapshots.java:
##########
@@ -116,6 +117,7 @@ public ExpireSnapshots cleanExpiredFiles(boolean clean) {
public ExpireSnapshots expireSnapshotId(long expireSnapshotId) {
LOG.info("Expiring snapshot with id: {}", expireSnapshotId);
idsToRemove.add(expireSnapshotId);
+ specifySnapshotIds = true;
Review Comment:
`specifiedSnapshotId`? Even though there can be multiple I don't think we need to emphasize that in the name, it doesn't make a difference in terms of the validation that gets 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 the behavior of IncrementalFileCleanup when expire a snapshot [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #10983:
URL: https://github.com/apache/iceberg/pull/10983#discussion_r1733882501
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -327,4 +342,34 @@ private Set<String> findFilesToDelete(
return filesToDelete;
}
+
+ private boolean isSafeToDelete(
+ ManifestEntry<?> entry,
+ Map<Long, Long> validSnapshotIdToSequenceNumberMap,
+ Map<Long, Long> expiredSnapshotIdToSequenceNumberMap) {
+ if (validSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())
+ || !expiredSnapshotIdToSequenceNumberMap.containsKey(entry.snapshotId())) {
+ return false;
+ }
+
+ // The file in DELETE entry can be deleted if there are no Snapshots older than
+ // this one
+ if (validSnapshotIdToSequenceNumberMap.keySet().stream()
+ .noneMatch(snapshotId -> snapshotId < entry.snapshotId())) {
+ return true;
Review Comment:
> Or potentially since we have the invariant here that there's only one ref at this point, we can just validate that there are no ancestors at all.
Thanks for providing this information. Based on the invariant you mentioned, we can make some useful decisions rely on the inheritance chain.
Firstly, if we find that the parent snapshot is still valid, we can determine directly that the file in DELETED entry cannot be safely deleted. It must exist in the parent snapshot.
Secondly, although we cannot determine whether all ancestors no longer exist by judging whether there all still valid ancestors, but conversely, we can confirm a situation that all the ancestors are expired in the same operation through checking the expired snapshots collection. This covers a narrower range, but the situation is quite common (due to the `expire older than` operation).
The advantage of rules relying on inheritance chain is that they can support both v1 and v2 tables.
And if the rules relying on inheritance chain do not work, we can take advantage of file sequence number to make some decisions for only v2 tables like what we do now, or maybe could figure out some other rules to do more intelligent decisions.
I have refactored the code to include the rules mentioned above, please take a look when available. Any informations or ideas will be greatly appreciated.
--
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