Skip to content

Core, API: Support cleanupMode in snapshot expiration #14287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stevenzwu merged 10 commits into from
Nov 21, 2025
Merged

Core, API: Support cleanupMode in snapshot expiration #14287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fda14eb
[Core,api] Support retainOrphanedDataFiles in snapshot expiration
dramaticlly Oct 8, 2025
ddd45ce
Introduce CleanupMode for snapshot expiration
dramaticlly Oct 17, 2025
dc0d38d
Rename to CleanupLevel and move as inner class
dramaticlly Oct 21, 2025
4ffe632
Reorder inner enum to be at the top
dramaticlly Oct 21, 2025
96288f0
Added cleanExpiredFiles back and throw better exception
dramaticlly Nov 17, 2025
22ad3ad
Remove id from ExpireSnapshots.CleanupLevel
dramaticlly Nov 18, 2025
61dbd6f
Revert "Remove id from ExpireSnapshots.CleanupLevel"
dramaticlly Nov 20, 2025
e27f60c
add explicit early termination
dramaticlly Nov 20, 2025
6f08f99
simplify
dramaticlly Nov 20, 2025
bf8f4c1
remove inheritDoc
dramaticlly Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions api/src/main/java/org/apache/iceberg/ExpireSnapshots.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,17 @@ public interface ExpireSnapshots extends PendingUpdate<List<Snapshot>> {
*/
ExpireSnapshots cleanExpiredFiles(boolean clean);

/**
* Skip the cleanup of orphaned data files as part of snapshot expiration
*
* @param retain true to retain orphaned data files only reachable by expired snapshots
* @return this for method chaining
*/
default ExpireSnapshots retainOrphanedDataFiles(boolean retain) {

@amogh-jahagirdar amogh-jahagirdar Oct 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this isn't necessary to achieve the behavior of "clean up the metadata files but keep the data/delete files". There's a deleteWith API which allows for custom functions to delete based on paths. A client could just pass in a function which based on if a path is a path in the metadata location clean it up, otherwise don't do anything.

@amogh-jahagirdar amogh-jahagirdar Oct 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the passed in logic can be inverted, to skip cleanup if it's in the data location or some other heuristic but I think the point still stands

@dramaticlly dramaticlly Oct 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @amogh-jahagirdar ! We actually explored that option and here's what we find

  1. use retainOrphanedDataFiles option actually speed up the clean up process by avoiding open and read the manifest files. If only metadata (like manifest-list and manifest) are considered for clean up, then we don't need to read the manifest entries, which is usually the bottleneck and requires work distribution. That's why most snapshot expiration is taken cared of in Spark action and procedures
  2. use DeleteWith consumer currently only provides a file path represented in String, we can use its file suffix to differentiate metadata and data files, but with introduction of Parquet Manifest - POC #13769, we can no longer rely on .parquet alone to tell. We can still probably rely on checking $tableLocation/data/ as part of file path but this is mostly conventional

@amogh-jahagirdar amogh-jahagirdar Oct 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dramaticlly , I mostly meant just using the table data location not a suffix as that wouldn't be sufficient but you're right that even the table data location is based on convention.
I agree though that specifying a custom cleanup function isn't the most ideal way to solve this use case.

I think the best argument for this kind of option is reducing costs (reducing unnecessary requests to read manifests, and the compute running when doing so) for cases where we know we only want to cleanup metadata and retain the data file.

The way I look at expressing this kind of logic is a bit different; rather than expressing which files we intend to retain, I look at it as which files should we cleanup. so something like a cleanExpiredFiles(CleanupMode mode)

and the options for CleanupMode like ALL, METADATA_ONLY, NONE. I think on this path we'd deprecate the cleanExpiredFiles(boolean) option as well, no point keeping both APIs?

If we were to go with the current approach we'd have to define precedence if someone uses the existing cleanExpiredFiles and the retainDataFiles, which is why it seems cleaner to me to define a cleanExpiredFiles API with some modes?

@dramaticlly dramaticlly Oct 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amogh-jahagirdar , I think what you proposed makes sense, as today retainDataFiles need to have prerequisites of cleanExpiredFiles=true.

But since those are public API on interface, deprecation and removal will need to go through cycles. I can update to use CleanupMode behind the scene.

throw new UnsupportedOperationException(
this.getClass().getName() + " doesn't implement retainOrphanedDataFiles");
}

/**
* Enable cleaning up unused metadata, such as partition specs, schemas, etc.
*
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,21 @@ public void accept(String file) {

protected final FileIO fileIO;
protected final ExecutorService planExecutorService;
protected final boolean retainDataFiles;
private final Consumer<String> deleteFunc;
private final ExecutorService deleteExecutorService;

protected FileCleanupStrategy(
FileIO fileIO,
ExecutorService deleteExecutorService,
ExecutorService planExecutorService,
Consumer<String> deleteFunc) {
Consumer<String> deleteFunc,
boolean retainDataFiles) {
this.fileIO = fileIO;
this.deleteExecutorService = deleteExecutorService;
this.planExecutorService = planExecutorService;
this.deleteFunc = deleteFunc;
this.retainDataFiles = retainDataFiles;
}

public abstract void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration);
Expand Down
17 changes: 11 additions & 6 deletions core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ class IncrementalFileCleanup extends FileCleanupStrategy {
FileIO fileIO,
ExecutorService deleteExecutorService,
ExecutorService planExecutorService,
Consumer<String> deleteFunc) {
super(fileIO, deleteExecutorService, planExecutorService, deleteFunc);
Consumer<String> deleteFunc,
boolean retainDataFiles) {
super(fileIO, deleteExecutorService, planExecutorService, deleteFunc, retainDataFiles);
}

@Override
Expand Down Expand Up @@ -251,11 +252,15 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
}
});

Set<String> filesToDelete =
findFilesToDelete(
manifestsToScan, manifestsToRevert, validIds, beforeExpiration.specsById());
if (!retainDataFiles) {
Set<String> filesToDelete =
findFilesToDelete(
manifestsToScan, manifestsToRevert, validIds, beforeExpiration.specsById());
LOG.debug("Deleting {} data files", filesToDelete.size());
deleteFiles(filesToDelete, "data");
}

deleteFiles(filesToDelete, "data");
LOG.debug("Deleting {} manifest files", manifestsToDelete.size());
Comment thread
dramaticlly marked this conversation as resolved.
deleteFiles(manifestsToDelete, "manifest");
Comment thread
dramaticlly marked this conversation as resolved.
deleteFiles(manifestListsToDelete, "manifest list");

Expand Down
14 changes: 10 additions & 4 deletions core/src/main/java/org/apache/iceberg/ReachableFileCleanup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ class ReachableFileCleanup extends FileCleanupStrategy {
FileIO fileIO,
ExecutorService deleteExecutorService,
ExecutorService planExecutorService,
Consumer<String> deleteFunc) {
super(fileIO, deleteExecutorService, planExecutorService, deleteFunc);
Consumer<String> deleteFunc,
boolean retainDataFiles) {
super(fileIO, deleteExecutorService, planExecutorService, deleteFunc, retainDataFiles);
}

@Override
Expand All @@ -72,10 +73,15 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
snapshotsAfterExpiration, deletionCandidates, currentManifests::add);

if (!manifestsToDelete.isEmpty()) {
Set<String> dataFilesToDelete = findFilesToDelete(manifestsToDelete, currentManifests);
deleteFiles(dataFilesToDelete, "data");
if (!retainDataFiles) {
Set<String> dataFilesToDelete = findFilesToDelete(manifestsToDelete, currentManifests);
LOG.debug("Deleting {} data files", dataFilesToDelete.size());
deleteFiles(dataFilesToDelete, "data");
}

Set<String> manifestPathsToDelete =
manifestsToDelete.stream().map(ManifestFile::path).collect(Collectors.toSet());
LOG.debug("Deleting {} manifest files", manifestPathsToDelete.size());
deleteFiles(manifestPathsToDelete, "manifest");
}
}
Expand Down
15 changes: 13 additions & 2 deletions core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class RemoveSnapshots implements ExpireSnapshots {
private Boolean incrementalCleanup;
private boolean specifiedSnapshotId = false;
private boolean cleanExpiredMetadata = false;
private boolean retainDataFiles = false;

RemoveSnapshots(TableOperations ops) {
this.ops = ops;
Expand Down Expand Up @@ -167,6 +168,12 @@ public ExpireSnapshots cleanExpiredMetadata(boolean clean) {
return this;
}

@Override
public ExpireSnapshots retainOrphanedDataFiles(boolean retain) {
this.retainDataFiles = retain;
return this;
}

@Override
public List<Snapshot> apply() {
TableMetadata updated = internalApply();
Expand Down Expand Up @@ -380,9 +387,13 @@ private void cleanExpiredSnapshots() {
FileCleanupStrategy cleanupStrategy =
incrementalCleanup
? new IncrementalFileCleanup(
ops.io(), deleteExecutorService, planExecutorService(), deleteFunc)
ops.io(), deleteExecutorService, planExecutorService(), deleteFunc, retainDataFiles)
: new ReachableFileCleanup(
ops.io(), deleteExecutorService, planExecutorService(), deleteFunc);
ops.io(),
deleteExecutorService,
planExecutorService(),
deleteFunc,
retainDataFiles);

cleanupStrategy.cleanFiles(base, current);
}
Expand Down
Loading