amogh-jahagirdar commented on code in PR #18408:
URL: https://github.com/apache/iceberg/pull/18408#discussion_r4243769283
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -131,6 +134,9 @@ public void cleanFiles(
try (CloseableIterable<ManifestFile> manifests =
readManifests(snapshot)) {
for (ManifestFile manifest : manifests) {
validManifests.add(manifest.path());
+ if (manifest.content() == ManifestContent.DELETES) {
+ liveDeleteManifests.putIfAbsent(manifest.path(),
manifest.copy());
Review Comment:
I think fixing this in incremental cleanup is fine for now. One alternative
we could consider is having RemoveSnapshots pick reachable cleanup when the
expired snapshots removed DVs. That's simpler to reason about and needs less
extra state, since reachable cleanup already covers this. The downside is that
reachable cleanup also reads all the data manifests, which are usually much
larger.
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -302,14 +314,21 @@ private Set<String> findFilesToDelete(
if (entry.status() == ManifestEntry.Status.DELETED
&& !validIds.contains(entry.snapshotId())) {
// use toString to ensure the path will not change (Utf8
is reused)
- filesToDelete.add(entry.file().location());
+ String location = entry.file().location();
+ filesToDelete.add(location);
+ if (entry.file().format() == FileFormat.PUFFIN) {
Review Comment:
Could we keep Puffin files out of filesToDelete until we know they're
unreferenced? Then the helper could just return the DV files that are safe to
delete, and we wouldn't need to add and then remove paths. I think that's a bit
easier to follow.
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -2144,6 +2144,70 @@ public void testCleanupLevelNullValidation() {
.hasMessageContaining("Invalid cleanup level: null");
}
+ @TestTemplate
+ public void testExpireDoesNotDeletePuffinFileSharedByLiveDeletionVectors()
throws Exception {
Review Comment:
Could we also add a test where all the DVs in a shared Puffin file get
replaced, and check that the file does get deleted after expiration? This test
would ensure that we still clean up Puffin files once nothing references them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
