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 any of
the expired snapshots reference dvs that have been DELETED. 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.
--
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]
