Async Deletion of Previous Metadata and Statistics Files #312

danielhumanmod · 2024-09-22T23:19:22Z

Description

Currently, when handling DROP TABLE PURGE, Polaris only deletes data files and the current metadata in TableCleanupTaskHandler. However, other related files, such as historical metadata.json files, stat files, and others, are not deleted.

Iceberg has addressed this issue (see Iceberg PR #9305), but Polaris doesn’t adopt this solution due to factors like asynchronous deletion. This PR introduces additional deletions for the following files:

Previous metadata files
Statistic files

Fixes #289

Included Changes

Updated TableCleanupTaskHandler.java to schedule previous metadata and statistics deletion in batch
Extended ManifestFileCleanupTaskHandler to delete metadata and statistics file batch
Extended TaskTestUtils.java to write metadata with previous metadata and statistics files
Add unit tests in TableCleanupTaskHandlerTest.java and TableContentCleanupTaskHandlerTest

Recommended Review Order:
TableCleanupTaskHandler.java -> ManifestFileCleanupTaskHandler.java -> TaskTestUtils.java -> TableCleanupTaskHandlerTest.java and ManifestFileCleanupTaskHandlerTest

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Unit Test:

TableCleanupTaskHandlerTest
ManifestFileCleanupTaskHandlerTest

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

…h purge

flyrain · 2024-09-24T00:27:44Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

+        tableMetadata.snapshots().stream()
+            .map(Snapshot::manifestListLocation)
+            .forEach(fileIO::deleteFile);


I think we've already handled the manifest files and manifest list files.

I think we've already handled the manifest files and manifest list files.

Thank you for bringing that up! I’ve just noticed that these are already handled in the ManifestFileCleanupTaskHandler. Have removed theses redundant deletion in the latest commit. Appreciate your input.

collado-mike · 2024-09-25T18:43:34Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

+        tableMetadata.previousFiles().stream()
+            .map(TableMetadata.MetadataLogEntry::file)
+            .forEach(fileIO::deleteFile);
+        tableMetadata.statisticsFiles().stream()
+            .map(StatisticsFile::path)
+            .forEach(fileIO::deleteFile);


We should either add retries for these or submit them as separate tasks. As is, if one of these files fails to delete, we'll retry the whole task and resubmit a new task for each manifest. If the manifests are already deleted when we retry, we'll get stuck in a retry loop.

Thanks for the suggestion, Michael! I could refactor the deletion process by adding retry strategy, similar to the tryDelete in ManifestFileCleanupTaskHandler.

Personally, I think using a retry mechanism here might be more effective than creating separate tasks. With separate tasks,, there’s still a risk of issues like task creation failures, which could result in skipping the entire task (which contains multiple files). By using retries within the ManifestFileCleanupTaskHandler, we can manage failure handling at the file level, ensuring that each file is retried independently. This way, if a file deletion fails, we can retry just that file without needing to resubmit or skip the others files. This approach can offers more granular control over handling failures.

I’m open to your thoughts on this! Does this seem aligned with what you were suggesting, or do you see potential advantages in the separate task approach that I might be overlooking?

Sorry, do you mean adding retry within the TableCleanupTaskHandler.java? If so, that's fine with me. I'd at least use an Executor to attempt the execution in parallel (i.e., so file2 isn't blocked during the delay between retries for file1).

TBH, I'm not very familiar with how Iceberg generates the statistic files. Does there tend to be one per snapshot? one per data file? If the latter, we could be talking about a very large number of files. If that's the case, I think submitting separate tasks with batches of files makes sense.

They are per snapshots, https://iceberg.apache.org/spec/#table-statistics.

Is there only one statistics file per snapshot? The spec is not clear:

A table can contain many statistics files associated with different table snapshots.

Unlike the partition statistics file, which is very clear:

Each table snapshot may be associated with at most one partition statistics file.

Could be multiple statistics files as the following code shows:

https://github.com/apache/iceberg/blob/5439cbdb278232779fdd9a392bbf57f007f9bda0/core/src/main/java/org/apache/iceberg/TableMetadata.java#L535-L535

They are per snapshots, https://iceberg.apache.org/spec/#table-statistics.

Could be multiple statistics files as the following code shows:

https://github.com/apache/iceberg/blob/5439cbdb278232779fdd9a392bbf57f007f9bda0/core/src/main/java/org/apache/iceberg/TableMetadata.java#L535-L535

Thank you for providing the context, @flyrain :)

Sorry, do you mean adding retry within the TableCleanupTaskHandler.java? If so, that's fine with me. I'd at least use an Executor to attempt the execution in parallel (i.e., so file2 isn't blocked during the delay between retries for file1).

TBH, I'm not very familiar with how Iceberg generates the statistic files. Does there tend to be one per snapshot? one per data file? If the latter, we could be talking about a very large number of files. If that's the case, I think submitting separate tasks with batches of files makes sense.

@collado-mike Yes, if we go for retries, the logic will be within TableCleanupTaskHandler.java. But considering the information provided by Yufei, maybe separate tasks will be a more appropriate approach for stats file deletion?

danielhumanmod · 2024-11-04T06:06:24Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

-        LOGGER.error("Unable to delete data files from manifest {}", manifestFile.path(), e);
-        return false;
-      }
+  private boolean cleanUpManifestFile(


Sorry for the lots of changes here, but don’t worry—it’s mainly because I refactored the deletion logic for the manifest and all its data into a new method; no other changes were made in lines 91-135.

danielhumanmod · 2024-11-04T06:18:16Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

    TableIdentifier tableId = cleanupTask.getTableId();
    try (FileIO authorizedFileIO = fileIOSupplier.apply(task)) {
-


Combine manifest and content file cleanup in the same task, but ensure that each task processes only one type of data (either manifest or content files) at a time.

danielhumanmod · 2024-11-04T06:29:29Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

+ * files in a manifest and the manifest itself. Since data files may be present in multiple
+ * manifests across different snapshots, we assume a data file that doesn't exist is missing because
+ * it was already deleted by another task. 2. Table content files: It contains previous metadata and
+ * statistics files, which are grouped and deleted in batch
 */
 public class ManifestFileCleanupTaskHandler implements TaskHandler {


Renaming this task will triger lots of relevent changes. If a rename is needed, we may want to handle it in a separate PR to avoid too much changes (Leave a TODO here)

danielhumanmod · 2024-11-05T05:28:40Z

...ervice/src/test/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandlerTest.java

@@ -226,4 +233,228 @@ public void deleteFile(String location) {
      assertThatPredicate((String f) -> TaskUtils.exists(f, fileIO)).rejects(dataFile2Path);
    }
  }
+


The test cases in this file primarily verify theses cleanup:

straightforward cleanup

cleanup when files do not exist

cleanup with retries

danielhumanmod · 2024-11-05T05:30:54Z

polaris-service/src/test/java/org/apache/polaris/service/task/TableCleanupTaskHandlerTest.java

The test cases in this file primarily verify theses scenarios:

Cleanup of a single metadata file with a single snapshot

Cleanup of a single metadata file across multiple snapshots

Cleanup of multiple metadata files across multiple snapshots (New)

danielhumanmod · 2024-11-05T05:31:10Z

polaris-service/src/test/java/org/apache/polaris/service/task/TableCleanupTaskHandlerTest.java

@@ -76,7 +79,13 @@ public void testTableCleanup() throws IOException {
      TestSnapshot snapshot =
          TaskTestUtils.newSnapshot(fileIO, "manifestList.avro", 1, snapshotId, 99L, manifestFile);
      String metadataFile = "v1-49494949.metadata.json";
-      TaskTestUtils.writeTableMetadata(fileIO, metadataFile, snapshot);


Cleanup of a single metadata file with a single snapshot

danielhumanmod · 2024-11-05T05:31:24Z

polaris-service/src/test/java/org/apache/polaris/service/task/TableCleanupTaskHandlerTest.java

@@ -303,7 +323,20 @@ public void testTableCleanupMultipleSnapshots() throws IOException {
              manifestFile1,
              manifestFile3); // exclude manifest2 from the new snapshot
      String metadataFile = "v1-295495059.metadata.json";
-      TaskTestUtils.writeTableMetadata(fileIO, metadataFile, snapshot, snapshot2);


Cleanup of a single metadata file across multiple snapshots

danielhumanmod · 2024-11-05T05:31:47Z

polaris-service/src/test/java/org/apache/polaris/service/task/TableCleanupTaskHandlerTest.java

+    }
+  }
+
+  @Test


Cleanup of multiple metadata files across multiple snapshots (New)

danielhumanmod · 2024-11-05T05:32:53Z

polaris-service/src/test/java/org/apache/polaris/service/task/TaskTestUtils.java

The changes in this file primarily attach previous metadata and statistics filewhen writing metadata

danielhumanmod · 2024-11-05T05:39:07Z

Thanks @danielhumanmod for working on it! left some coments.

Thanks for the suggestions @flyrain , I've updated the code based on your comments and added some comment to help the code easier to understand. I’d appreciate any further feedback, and thanks for your time and patience!

flyrain

Sorry for the delay, @danielhumanmod. Left some comments.

flyrain · 2024-11-12T06:22:54Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

        fileIO.deleteFile(tableEntity.getMetadataLocation());

        return true;
      }
    }
    return false;
  }
+
+  private List<List<String>> getContentFileBatch(TableMetadata tableMetadata, FileIO fileIO) {


The name contentFile in Iceberg refers to a data file or a delete file. Let's pick another name to avoid confusion, as metadata files and statistic files are not content files. How about metadataFile?

flyrain · 2024-11-12T06:24:21Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

+
+  private List<List<String>> getContentFileBatch(TableMetadata tableMetadata, FileIO fileIO) {
+    List<List<String>> result = new ArrayList<>();
+    List<String> contentFiles =


rename here

flyrain · 2024-11-12T06:32:03Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

+        Stream.concat(
+                tableMetadata.previousFiles().stream().map(TableMetadata.MetadataLogEntry::file),
+                tableMetadata.statisticsFiles().stream().map(StatisticsFile::path))
+            .filter(file -> TaskUtils.exists(file, fileIO))


Do we need to check the file existence here? It will be checked at delete time, right?

flyrain · 2024-11-12T06:36:07Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

-              .toList();
+                  });
+
+      Stream<TaskEntity> contentFileCleanupTasks =


-> metadataFileCleanupTask

flyrain · 2024-11-12T06:43:39Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

@@ -102,7 +106,7 @@ public boolean handleTask(TaskEntity cleanupTask) {
      // read the manifest list for each snapshot. dedupe the manifest files and schedule a
      // cleanupTask
      // for each manifest file and its data files to be deleted
-      List<TaskEntity> taskEntities =


This method is quite long. Can we split it to multiple methods like this?

var manifestFileCleanupTask = getTaskStream(...); var metadataFileCleanupTasks = getTaskEntityStream(cleanupTask, tableMetadata, fileIO, tableEntity, metaStoreManager, polarisCallContext); var taskEntities = Stream.concat(manifestCleanupTasks, contentFileCleanupTasks).toList();

flyrain · 2024-11-12T06:45:35Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

@@ -185,12 +236,18 @@ private CompletableFuture<Void> tryDelete(
  public static final class ManifestCleanupTask {
    private TableIdentifier tableId;
    private String manifestFileData;
+    private List<String> contentFileBatch;


rename here

flyrain · 2024-11-12T06:53:40Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

+      LOGGER
+          .atWarn()
+          .addKeyValue("contentFileBatch", contentFileBatch.toString())
+          .addKeyValue("tableId", tableId)
+          .log("Table content cleanup task scheduled, but the none of the file in batch exists");
+      return true;


Do we need to warn here if 1 out of 10 files doesn't exist?

flyrain · 2024-11-12T06:57:15Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

+    LOGGER
+        .atInfo()
+        .addKeyValue("contentFileBatch", contentFileBatch.toString())
+        .addKeyValue("tableId", tableId)
+        .log("Content file batch deletion has completed");


We can either remove it or make it debug level. An info level log may not be necessary.

flyrain · 2024-11-12T06:59:00Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

+      LOGGER
+          .atWarn()
+          .addKeyValue("contentFileBatch", contentFileBatch.toString())
+          .addKeyValue("tableId", tableId)
+          .log("Exception detected during content file batch deletion", e);


Should we throw here?

ajantha-bhat · 2024-11-12T07:29:48Z

...is-service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java

+ * files in a manifest and the manifest itself. Since data files may be present in multiple
+ * manifests across different snapshots, we assume a data file that doesn't exist is missing because
+ * it was already deleted by another task. 2. Table content files: It contains previous metadata and
+ * statistics files, which are grouped and deleted in batch


Need to handle the same for partition statistics files also (in this PR or as a follow up)
more details: apache/iceberg#9409

Need to handle the same for partition statistics files also (in this PR or as a follow up) more details: apache/iceberg#9409

Thanks for your suggestion! Would it be okay to address this in a separate PR avoid making this one too large? (Left a comment on the code, I can create an issue to track it if necessary)

works for me.

- 1. renaming - 2. add log and exception handling - 3. remove unnecessary log

- 1. renaming - 2. extract task entities creation into methods - 3. remove unnecessary filtering

…to pr-289

danielhumanmod · 2024-11-14T06:37:27Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

        fileIO.deleteFile(tableEntity.getMetadataLocation());

        return true;
      }
    }
    return false;
  }
+


Extract previous manifest task creation into a new method, no new change added for line 152 - 201

danielhumanmod · 2024-11-14T06:38:06Z

polaris-service/src/main/java/org/apache/polaris/service/task/TableCleanupTaskHandler.java

+                  .build();
+            });
+  }
+


The code blow this line is the new changes

danielhumanmod · 2024-11-14T06:44:24Z

Thanks for your review and suggestions @flyrain @ajantha-bhat ! I've made updates to the code based on your comments and would greatly appreciate any additional feedback. I understand this PR is a bit long—thank you again for your patience and time!

danielhumanmod added 7 commits September 22, 2024 15:48

delete manifest, manifest list, prev files, stats when drop table wit…

d0cd456

…h purge

unit test for drop table

26e03ac

refine warning code

0f8e8f4

code format

1b525de

refine warning code

e8b26d2

remove unused code

2ee6dee

remove unused import

806f46d

danielhumanmod mentioned this pull request Sep 22, 2024

DROP TABLE with PURGE does not delete metadata.json files #289

Open

1 task

code format

4f1d3c9

flyrain reviewed Sep 24, 2024

View reviewed changes

danielhumanmod added 3 commits September 23, 2024 22:49

remove additional manifest and manifest list deletion

0a77bfa

add stat deletion test

47dc60a

code format

9d835b3

danielhumanmod marked this pull request as ready for review September 24, 2024 06:03

danielhumanmod requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau and jackye1995 as code owners September 24, 2024 06:03

danielhumanmod requested a review from flyrain September 24, 2024 06:03

danielhumanmod changed the title ~~Delete all related files in TableCleanUpTask~~ Delete prev metadata and stat files in TableCleanUpTask Sep 24, 2024

collado-mike requested changes Sep 25, 2024

View reviewed changes

danielhumanmod added 5 commits October 6, 2024 00:20

add new AsyncTaskType

40c6147

Schedule prev metadata and stat files deletion in seperated tasks

f354d1c

Table content cleanup task handler

88c6651

Unit test for table clean up

af3efab

code format

278ab7e

danielhumanmod requested a review from eric-maynard as a code owner October 6, 2024 07:33

danielhumanmod and others added 4 commits November 3, 2024 21:46

Merge branch 'main' into pr-289

e92852e

merge cleanup tasks into one

27ea1b3

Merge remote-tracking branch 'origin/pr-289' into pr-289

4d1b68b

code format

eb533d7

danielhumanmod commented Nov 4, 2024

View reviewed changes

danielhumanmod commented Nov 5, 2024

View reviewed changes

danielhumanmod requested a review from flyrain November 5, 2024 05:39

Merge branch 'main' into pr-289

47f760f

flyrain requested a review from adutra as a code owner November 9, 2024 05:45

flyrain reviewed Nov 12, 2024

View reviewed changes

ajantha-bhat reviewed Nov 12, 2024

View reviewed changes

danielhumanmod added 4 commits November 13, 2024 22:00

refactor manifest cleanup handler based on comments

988e530

- 1. renaming - 2. add log and exception handling - 3. remove unnecessary log

refactor table cleanup handler based on comments

4965d5c

- 1. renaming - 2. extract task entities creation into methods - 3. remove unnecessary filtering

add TODO

5f81483

Merge branch 'pr-289' of https://github.com/danielhumanmod/polaris in…

097189c

…to pr-289

danielhumanmod commented Nov 14, 2024

View reviewed changes

danielhumanmod requested review from ajantha-bhat and flyrain November 14, 2024 06:41

danielhumanmod and others added 2 commits November 13, 2024 22:44

Merge branch 'main' into pr-289

651ece0

renaming

16bb5fe

		TableIdentifier tableId = cleanupTask.getTableId();
		try (FileIO authorizedFileIO = fileIOSupplier.apply(task)) {

+                  }
+                }
+                @Test

Async Deletion of Previous Metadata and Statistics Files #312

Are you sure you want to change the base?

Async Deletion of Previous Metadata and Statistics Files #312

Conversation

danielhumanmod commented Sep 22, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

flyrain Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

danielhumanmod Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

danielhumanmod commented Nov 5, 2024

flyrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhumanmod commented Nov 14, 2024

danielhumanmod commented Sep 22, 2024 •

edited

Loading

flyrain Sep 24, 2024 •

edited

Loading

danielhumanmod Sep 24, 2024 •

edited

Loading

danielhumanmod Sep 26, 2024 •

edited

Loading

danielhumanmod Nov 4, 2024 •

edited

Loading

danielhumanmod Nov 4, 2024 •

edited

Loading

danielhumanmod Nov 5, 2024 •

edited

Loading

danielhumanmod Nov 14, 2024 •

edited

Loading

danielhumanmod Nov 14, 2024 •

edited

Loading