1 delete orphan files by subkanthi · Pull Request #35 · Altinity/ice

subkanthi · 2025-06-23T11:47:22Z

closes:#1

…enace schedule

…enance schedule

shyiko · 2025-06-23T21:24:06Z

+
+            // Remove orphans.
+            OrphanFileScanner orphanFileScanner = new OrphanFileScanner(table);
+            orphanFileScanner.removeOrphanedFiles(olderThanMillis, false);


we may want a shorter ttl here

shyiko · 2025-06-23T21:34:55Z

+import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
+
+public class OrphanFileScanner {
+  private static final Logger LOG = LoggerFactory.getLogger(OrphanFileScanner.class);


called logger in the rest of the codebase

shyiko · 2025-06-23T21:38:22Z

+    String bucket = location.replace("s3://", "").split("/")[0];
+    String prefix = location.replace("s3://" + bucket + "/", "");
+
+    S3Client s3 = S3.newClient(true);


com.altinity.ice.cli.internal.* shouldn't be used here; true indicates "anonymous" access -> won't work;
can we use table io here (so that impl would be not tied to s3)?

shyiko · 2025-06-23T21:43:41Z

+        deletedFiles.forEach(f -> LOG.info("Deleted: {}", f));
+      }
+
+      executor.shutdownNow();


we already called shutdown on line 120

shyiko · 2025-06-23T21:48:06Z

+    return allFiles;
+  }
+
+  public void removeOrphanedFiles(long olderThanMillis, boolean dryRun) throws IOException {


based on current impl this method should be executed only when warehouse is s3 (not s3 tables, etc)

shyiko · 2025-06-23T21:53:41Z

+
+    // Set<String> orphanedFiles = new HashSet<>();
+
+    allFiles.remove(knownFiles);


isn't this supposed to be removeAll?

shyiko · 2025-06-23T22:00:18Z

+      }
+    }
+
+    return knownFiles;


The result looks incomplete. Inserting a single file into empty table yields

./metadata/00000-f62c927f-3965-42e9-8f3a-57809ca49f55.metadata.json ./metadata/4b00c8c2-6d25-4ba5-bf27-989fbdb9daa1-m0.avro ./metadata/snap-7186194791324723249-1-4b00c8c2-6d25-4ba5-bf27-989fbdb9daa1.avro ./metadata/00001-07fe7fa7-8fe1-4666-bd7b-7c88b9c861fe.metadata.json ./data/pickup_datetime_month=2009-01/1750715797525-2d861d7a4e5196bf04e5c3be506b0152f4170cf4e941424c8245b46903c3e252-part.parquet

i.e. we're going to nuke a bunch of critical files and break the catalog...

shyiko · 2025-06-25T21:42:23Z

+            knownFiles.add(dataFile.path().toString());
+          }
+        } catch (Exception e) {
+          logger.error("Error getting list of data files", e);


network error here leads to us deleting files from catalog that are not supposed to be deleted

…-files

subkanthi · 2025-07-03T16:31:53Z

…eve snapshot metadata JSON files for orphan file deletion.

subkanthi · 2025-07-04T00:22:24Z

Tested by dropping a new file

2025-07-03 19:17:03 [-1-thread-1] INFO c.a.i.r.c.i.m.OrphanFileScanner > Looking for Orphaned files in location s3://bucket1/flowers/iris
2025-07-03 19:21:37 [-1-thread-1] INFO c.a.i.r.c.i.m.OrphanFileScanner > Found 1 orphaned files at s3://bucket1/flowers/iris!
2025-07-03 19:21:37 [-1-thread-1] INFO c.a.i.r.c.i.m.OrphanFileScanner > Deleted 1 orphaned files at s3://bucket1/flowers/iris!
2025-07-03 19:21:37 [-1-thread-1] INFO c.a.i.r.c.i.m.OrphanFileScanner > Deleted: s3://bucket1/flowers/iris/metadata/restore.sql

subkanthi · 2025-07-04T00:25:02Z

+      return;
+    }
+
+    if (dryRun) {


dryRun should probably a config variable, so they can try before toggling it?

subkanthi · 2025-07-04T00:25:52Z

+
+            if (orphanFileExpirationDays == 0) {
+              logger.info(
+                  "Skipping orphan file removal for table {} since orphanFileExpirationDays config was not set",


maybe this can be avoided for every table if we have a separate iteration, not sure if this is the best option.

shyiko · 2025-07-09T06:13:57Z

we should probably default to

Unreferenced file removal identifies and deletes all objects that are not referenced by any table snapshots. As part of your unreferenced file removal policy, you can configure two properties: unreferencedDays (3 days by default) and nonCurrentDays (10 days by default).

to match https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-table-buckets-maintenance.html

subkanthi · 2025-07-09T22:34:09Z

we should probably default to

Unreferenced file removal identifies and deletes all objects that are not referenced by any table snapshots. As part of your unreferenced file removal policy, you can configure two properties: unreferencedDays (3 days by default) and nonCurrentDays (10 days by default).

to match https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-table-buckets-maintenance.html

Setting default to 10 days for orphanFileExpirationDayssimilar to unreferencedDays
This functionality nonCurrentDays is not implemented, the files are removed after orphanFileExpirationDays

shyiko · 2025-07-10T20:05:37Z

+      FileIO io = table.io();
+
+      TableOperations ops = ((BaseTable) table).operations();
+      TableMetadata meta = ops.current();
+
+      String currentMetadataFile = meta.metadataFileLocation();
+      // Current metadata json file
+      knownFiles.add(currentMetadataFile);
+
+      // All the previous metadata JSON(is there a chance there might be
+      // json files that are not physically present?.
+      for (TableMetadata.MetadataLogEntry previousFile : meta.previousFiles()) {
+        knownFiles.add(previousFile.file());
+      }


same files are added on each loop iteration

which ones, I dont see the same files

you have a single snapshot; add another one to repro. but take a look at the selected code - it's fairly obvious the code is not connected to the outer for loop

shyiko · 2025-07-11T16:16:14Z

+          for (DataFile dataFile : files) {
+            knownFiles.add(dataFile.path().toString());
+          }
+        } catch (Exception e) {


nit: catch/throw

shyiko · 2025-07-11T16:17:02Z

+      // All the previous metadata JSON(is there a chance there might be
+      // json files that are not physically present?.
+      for (TableMetadata.MetadataLogEntry previousFile : meta.previousFiles()) {
+        knownFiles.add(previousFile.file());


the result is missing statistics & partition statistics files

shyiko · 2025-07-11T16:18:45Z

+
+      // All the previous metadata JSON(is there a chance there might be
+      // json files that are not physically present?.
+      for (TableMetadata.MetadataLogEntry previousFile : meta.previousFiles()) {


files referenced by previous manifests are also missing

shyiko · 2025-08-18T17:12:06Z

A version of this with #35 (review) addressed was included in 0.5.0.

subkanthi added 2 commits June 22, 2025 08:23

Added logic to scan for orphan files and remove them as part of maint…

2fed100

…enace schedule

Added logic to scan for orphan files and remove them as part of maint…

483fdfa

…enance schedule

subkanthi linked an issue Jun 23, 2025 that may be closed by this pull request

ice-rest-catalog: Automate catalog maintenance #1

Closed

Replaced HadoopFileIO with S3FileIO

0214f29

shyiko requested changes Jun 23, 2025

View reviewed changes

Addressed PR review comments.

b3ee8d3

shyiko reviewed Jun 25, 2025

View reviewed changes

Merge branch 'master' of github.com:Altinity/ice into 1-delete-orphan…

a2dea79

…-files

subkanthi added 3 commits July 3, 2025 16:49

Created new config for orphanFileExpiration TTL. Added logic to retri…

ac4455f

…eve snapshot metadata JSON files for orphan file deletion.

Fixed info message.

f5b068f

Fixed info message.

c891d38

subkanthi marked this pull request as ready for review July 4, 2025 00:22

subkanthi requested a review from shyiko July 4, 2025 00:22

subkanthi commented Jul 4, 2025

View reviewed changes

set default orphan expiration days to 10

68d27a4

shyiko requested changes Jul 11, 2025

View reviewed changes

shyiko closed this Aug 18, 2025


		// Set<String> orphanedFiles = new HashSet<>();

		allFiles.remove(knownFiles);

Conversation

subkanthi commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subkanthi commented Jul 3, 2025

Uh oh!

subkanthi commented Jul 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shyiko commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subkanthi commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shyiko Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shyiko commented Aug 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

subkanthi commented Jun 23, 2025 •

edited

Loading

shyiko commented Jul 9, 2025 •

edited

Loading

subkanthi commented Jul 9, 2025 •

edited

Loading

shyiko Jul 11, 2025 •

edited

Loading