Core: Expire Snapshots reachability analysis by amogh-jahagirdar · Pull Request #5669 · apache/iceberg

amogh-jahagirdar · 2022-08-29T20:49:22Z

Currently, Snapshot expiration has a limitation which prevents file cleanup from being performed if there are multiple branches and tags. That's because for reliable file cleanup in the presence of multiple references, a reachability analysis needs to be performed to determine which files can safely be removed. The existing incremental file cleanup cannot be performed in this case.

This PR introduces a FileCleanupStrategy which is a base strategy pattern that classes override with their own file cleanup logic. There are 2 strategy implementations

1.) IncrementalFileCleanup which is used in RemoveSnapshots procedure in the case there's only 1 reference. This is simply the existing file cleanup logic.
2.) A new ReachableFileCleanup which performs a reachability analysis of reachable manifest lists, manifests, and data files given the previous and current table states.

Closes#5666.

amogh-jahagirdar · 2022-08-29T20:51:49Z

Still very incomplete, just starting a draft.

amogh-jahagirdar · 2022-09-04T21:53:13Z

cc: @rdblue @namrathamyske @singhpk234 @jackye1995 I moved it out of draft state, would be great to get your thoughts on this. Thanks!

amogh-jahagirdar · 2022-10-01T21:37:02Z

+ // Reads and deletes are done using Tasks.foreach(...).suppressFailureWhenFinished to complete
+ // as much of the delete work as possible and avoid orphaned data or manifest files.
+ SnapshotRef branchToCleanup = Iterables.getFirst(base.refs().values(), null);
+ if (branchToCleanup == null) {
+ return;
+ }


Porting the fix from https://github.com/apache/iceberg/pull/5666/files. This is still needed if we want to support incremental file cleanups in the case that expiration is called on a table with only non-main commits.

amogh-jahagirdar · 2022-10-01T21:39:51Z

+
+ protected void deleteMetadataFiles(
+ Set<String> manifestsToDelete, Set<String> manifestListsToDelete) {
+ log.warn("Manifests to delete: {}", Joiner.on(", ").join(manifestsToDelete));


https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L538
This is still warn as it is currently, but I'm doubting if it really needs to be warn level or if we even need this logging? warn level implies that something is in a "not fatally wrong" which is not the case here. We are intentionally deleting these files after they've been correctly computed. It may mislead users. @rdblue @singhpk234 @jackye1995

rdblue · 2022-10-09T19:07:30Z

+ private Set<ManifestFile> readManifests(Set<Snapshot> snapshots) {
+ Set<ManifestFile> manifestFiles = Sets.newHashSet();
+ for (Snapshot snapshot : snapshots) {
+ try (CloseableIterable<ManifestFile> manifestFilesForSnapshot = readManifestFiles(snapshot)) {


We may want to do this in parallel using Tasks. The results are copied anyway so there isn't much of a cost to it.

Oh missed this when addressing the comments, is this blocking or could I address in a follow on PR along with https://github.com/apache/iceberg/pull/5669/files#r990877250 ?

We can do it in a follow up.

In fact, we may want to do something like what you do in findFilesToDelete, where rather than reading a set of current manifest files, you just remove manifests from the candidate set. You could also use the same logic to short circuit if there are no more candidate manifests.

rdblue

@amogh-jahagirdar, this looks very good. There's just one blocker here: #5669 (comment). There are some minor things as well you may want to clean up but once the blocker is in we can commit this. Thank you!

zinking · 2022-10-12T06:32:15Z

+ @Override
+ @SuppressWarnings({"checkstyle:CyclomaticComplexity", "MethodLength"})
+ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpiration) {
+ if (afterExpiration.refs().size() > 1) {


this method seems way too long, wasn't it better to refactor into several private methods with names explaining the intents ?

@zinking, that's a great idea, but not something we should do in this PR. This code was moved from RemoveSnapshots so we want to keep it as close to the original as possible. Would you like to follow up with a PR to simplify it and get rid of the checkstyle warning?

Yeah agreed @zinking as @rdblue mentioned, in this PR we just opted for keeping the same logic for incremental cleanup that existed in the previous RemoveSnapshots implementation. Would be great to refactor this and remove the checkstyle warning

rdblue · 2022-10-12T23:40:08Z

+ .onFailure(
+ (item, exc) ->
+ LOG.warn(
+ "Failed to determine live files in manifest {}: this may cause orphaned data files",


This isn't true anymore. It should state that there will be a retry.

rdblue · 2022-10-12T23:44:29Z

+ });
+
+ } catch (Throwable e) {
+ LOG.warn("Failed to determine the data files to be removed", e);


As a follow up, I think this message could be a little better. This makes sense if you know what the code is currently doing, but if you don't then I think it will be hard to understand that the error was in reading current manifests, rather than dropped manifests. I'd use something like "Failed to list all reachable files".

rdblue · 2022-10-12T23:45:00Z

Thanks, @amogh-jahagirdar! This looks good.

rdblue · 2022-10-12T23:45:51Z

FYI @puneetzaroo and @szehon-ho, this affects incremental table cleanup.

github-actionsBot added the core label Aug 29, 2022

amogh-jahagirdar force-pushed the expire-snapshots-reachability branch from a84f92d to 7e23bd3Compare August 29, 2022 20:51

amogh-jahagirdar force-pushed the expire-snapshots-reachability branch 5 times, most recently from 8fcb2bc to a6807aaCompare August 30, 2022 00:11

amogh-jahagirdar commented Aug 30, 2022
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/RemoveSnapshots.java Outdated

amogh-jahagirdar force-pushed the expire-snapshots-reachability branch 2 times, most recently from b9525c7 to be9232cCompare August 30, 2022 03:27

amogh-jahagirdar commented Aug 30, 2022
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/RemoveSnapshots.java Outdated

amogh-jahagirdar force-pushed the expire-snapshots-reachability branch 6 times, most recently from c9e1ab8 to 302b9ebCompare September 4, 2022 05:51

amogh-jahagirdar marked this pull request as ready for review September 4, 2022 05:51

amogh-jahagirdar force-pushed the expire-snapshots-reachability branch 3 times, most recently from 324fe11 to 49021caCompare September 4, 2022 06:04

amogh-jahagirdar commented Sep 4, 2022
View reviewed changes

Comment threadcore/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java

amogh-jahagirdar mentioned this pull request Sep 13, 2022
Performing file deletion in ExpireSnapshots with Branching and Tagging #5653
Closed

singhpk234 reviewed Sep 29, 2022
View reviewed changes

amogh-jahagirdar force-pushed the expire-snapshots-reachability branch 2 times, most recently from 95cd087 to 8698582Compare October 1, 2022 21:32

amogh-jahagirdar commented Oct 1, 2022
View reviewed changes