Improve file delete performance for expire_snapshots #26230

tbaeg · 2025-07-17T15:28:36Z

Description

Improve file delete performance for expire_snapshots.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## Iceberg
* Improve performance of expire_snapshots procedure. ({issue}`26230`)

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

.../trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java

Copilot

Pull Request Overview

This PR improves the performance of the expire_snapshots procedure in Iceberg by introducing parallel file deletion capabilities and delegating file cleanup operations to Iceberg's native bulk delete functionality.

Key changes include:

Introduces a new configurable thread pool for file deletion operations
Replaces custom file deletion logic with Iceberg's native bulk delete operations
Updates thread pool sizing defaults with reasonable upper bounds

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
ForIcebergFileDelete.java	Adds new binding annotation for file delete executor dependency injection
IcebergConfig.java	Adds configuration for file delete thread pool size
IcebergExecutorModule.java	Creates new executor service for file deletion operations
IcebergMetadata.java	Replaces custom file deletion with Iceberg's native bulk delete operations
IcebergMetadataFactory.java	Integrates file delete executor into metadata factory
TestIcebergConfig.java	Updates tests for new configuration and thread pool defaults
BaseIcebergMinioConnectorSmokeTest.java	Updates test expectations for bulk delete behavior
Various test files	Adds file delete executor parameter to test constructors

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

raunaqmorarka · 2025-07-19T23:57:59Z

@tbaeg are you able to share any benchmark results from trying this change ?

tbaeg · 2025-07-20T00:15:46Z

@tbaeg are you able to share any benchmark results from trying this change ?

At the moment, I do not have any benchmarks to share. For our testing environment I'd first need to back port the change. I can try to get something this week.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

ebyhr

Looks good except for missing documentation.

findinpath · 2025-07-21T17:32:06Z

@tbaeg if it is not much trouble, could you add in the description of the PR an informal benchmark with and without parallelism for expire_snapshot.
I'd be interested to see the order of acceleration in terms of time we can achieve when using parallelism.

findinpath · 2025-07-21T17:33:43Z

docs/src/main/sphinx/connector/iceberg.md

@@ -228,6 +228,9 @@ implementation is used:
  - Number of threads used for retrieving metadata. Currently, only table loading 
    is parallelized.
  - `8`
+* - `iceberg.file-delete-threads`
+  - Number of threads to use for deleting files when running `expire_snapshots` procedure.
+  - Double the number of processors on the coordinator node.


Misc question: Do we have any way to restrict certain settings only for the coordinator ?

Outside of the scope of the current PR

.../trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java

findinpath · 2025-07-21T17:49:56Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+        // ForwardingFileIo handles bulk operations so no separate function implementation is needed
+        table.expireSnapshots()
+                .expireOlderThan(session.getStart().toEpochMilli() - retention.toMillis())
+                .executeDeleteWith(fileDeleteExecutor)


i wonder whether we could ensure somehow that there are no regressions in terms of sending in bulk requests to aws s3

Assuming something needs to be deleted in each category (i.e. - data/manifest/manifest list/statistics) you technically have more minimum requests. But the improvements should really shine as more files need to be deleted. So in a sense, this is a "regression".. although I'd categorize this as more of a trade off for throughput.

One of the aspects I imagine could become an issue, is excessive 500 responses from S3, since each element in a batch delete requests technically translates to a request towards the rate limiting quota. This would require testing at a certain scale to verify.

Apologies, I was wrong about the parallelization in iceberg. Looking at the code more closely, it only parallelizes when the FileIO implementation does NOT support bulk operations. So, there is effectively no performance change.

There would need to be a change in iceberg to support parallelized bulk operations, or the ForwardingFileIo would need to handle the parallelization.

Old: 10 per deletes per request (manually set the delete batch size) - 412 total requests 2025-07-21T13:00:29.337-0600 INFO ForkJoinPool-1-worker-1 stdout 5024 metadata files found 2025-07-21T13:00:29.338-0600 INFO ForkJoinPool-1-worker-1 stdout 1002 data files found 2025-07-21T13:00:32.795-0600 INFO ForkJoinPool-1-worker-1 stdout Time for expire_snapshots time: 3457 2025-07-21T13:00:32.845-0600 INFO ForkJoinPool-1-worker-1 stdout 2011 metadata files found after expire_snapshots 2025-07-21T13:00:32.845-0600 INFO ForkJoinPool-1-worker-1 stdout 2 data files found after expire_snapshots New: 10 per deletes per request (manually set the delete batch size) - 413 total requests 2025-07-21T12:53:18.326-0600 INFO ForkJoinPool-1-worker-1 stdout 5024 metadata files found 2025-07-21T12:53:18.326-0600 INFO ForkJoinPool-1-worker-1 stdout 1002 data files found 2025-07-21T12:53:21.775-0600 INFO ForkJoinPool-1-worker-1 stdout Time for expire_snapshots time: 3448 2025-07-21T12:53:21.819-0600 INFO ForkJoinPool-1-worker-1 stdout 2011 metadata files found after expire_snapshots 2025-07-21T12:53:21.819-0600 INFO ForkJoinPool-1-worker-1 stdout 2 data files found after expire_snapshots

In our case SupportsBulkOperations is always implemented because of using ForwardingFileIo.
So we have to do the parallelism in io.trino.plugin.iceberg.fileio.ForwardingFileIo#deleteFiles and skip using executeDeleteWith.

Another crude benchmark.

2025-07-22T12:50:13.288-0600 INFO ForkJoinPool-1-worker-1 stdout 5024 metadata files found 2025-07-22T12:50:13.289-0600 INFO ForkJoinPool-1-worker-1 stdout 1002 data files found 2025-07-22T12:50:15.741-0600 INFO ForkJoinPool-1-worker-1 stdout Time for expire_snapshots time: 2452 2025-07-22T12:50:15.797-0600 INFO ForkJoinPool-1-worker-1 stdout 2011 metadata files found after expire_snapshots 2025-07-22T12:50:15.797-0600 INFO ForkJoinPool-1-worker-1 stdout 2 data files found after expire_snapshots

tbaeg · 2025-07-21T18:19:01Z

@tbaeg if it is not much trouble, could you add an informal benchmark with and without parallelism for expire_snapshot. I'd be interested to see the order of acceleration in terms of time we can achieve when using parallelism.

I was trying to get something informal together on a test cluster with more substantial data, but it looks like it might not be possible. I can write a simple test and profile it locally, but not sure how worthwhile that is.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergExecutorModule.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

tbaeg · 2025-07-22T16:53:01Z

Did a POC that parallelizes ForwardingFileIo.deleteFiles and introduced a new FileIoFactory so I didn't need to inject the ExecutorService every time a new ForwardingFileIo object was created. If you could take a look again, I'd appreciate it.

@raunaqmorarka @findinpath @chenjian2664 @ebyhr

raunaqmorarka

.

raunaqmorarka · 2025-07-23T09:28:40Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/FileIoModule.java

+import com.google.inject.Module;
+import com.google.inject.Scopes;
+
+public class FileIoModule


Why do we need a new Module now ? Doesn't seem justified with just one binding

Mostly because the lakehouse connector was using it. If I place it in the IcebergModule it doesn't work. Open to suggestions on where to place it.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/FileIoFactory.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java

...in/trino-iceberg/src/main/java/io/trino/plugin/iceberg/procedure/RegisterTableProcedure.java

- Add ForwardingFileIoFactory so delete specific ExecutorService is not injected in multiple places - Remove direct usage of ForwardingFileIo in favor of a FileIoFactory implementation - Add parallelized deletes in ForwardingFileIo

cla-bot bot added the cla-signed label Jul 17, 2025

github-actions bot added the iceberg Iceberg connector label Jul 17, 2025

tbaeg force-pushed the expire-snapshots-parallel branch from 9e77d23 to 9ffdd87 Compare July 17, 2025 16:42

tbaeg requested a review from raunaqmorarka July 18, 2025 20:17

raunaqmorarka reviewed Jul 18, 2025

View reviewed changes

raunaqmorarka requested review from ebyhr, mayankvadariya and findinpath July 18, 2025 21:04

raunaqmorarka added the performance label Jul 18, 2025

tbaeg force-pushed the expire-snapshots-parallel branch 2 times, most recently from 896fd0d to e0cd470 Compare July 19, 2025 14:16

raunaqmorarka approved these changes Jul 19, 2025

View reviewed changes

raunaqmorarka requested review from sopel39 and Copilot July 19, 2025 15:57

Copilot AI reviewed Jul 19, 2025

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java Show resolved Hide resolved

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Show resolved Hide resolved

mayankvadariya approved these changes Jul 20, 2025

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

ebyhr reviewed Jul 21, 2025

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java Show resolved Hide resolved

ebyhr approved these changes Jul 21, 2025

View reviewed changes

tbaeg force-pushed the expire-snapshots-parallel branch from e0cd470 to c0402c7 Compare July 21, 2025 13:01

github-actions bot added the docs label Jul 21, 2025

findinpath reviewed Jul 21, 2025

View reviewed changes

.../trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java Outdated Show resolved Hide resolved

tbaeg force-pushed the expire-snapshots-parallel branch from c0402c7 to 4dc4d6c Compare July 21, 2025 17:43

findinpath reviewed Jul 21, 2025

View reviewed changes

tbaeg force-pushed the expire-snapshots-parallel branch from 4dc4d6c to 5bc97de Compare July 21, 2025 17:53

chenjian2664 reviewed Jul 22, 2025

View reviewed changes

tbaeg force-pushed the expire-snapshots-parallel branch 4 times, most recently from dc1c0d2 to 8845baa Compare July 22, 2025 16:44

tbaeg force-pushed the expire-snapshots-parallel branch 4 times, most recently from 353c0eb to 49a2a12 Compare July 22, 2025 18:46

raunaqmorarka mentioned this pull request Jul 23, 2025

Delete operation is not executed with ThreadPools#DELETE_WORKER_POOL apache/iceberg#12590

Open

3 tasks

raunaqmorarka reviewed Jul 23, 2025

View reviewed changes

tbaeg force-pushed the expire-snapshots-parallel branch 2 times, most recently from 8685530 to d39a0a4 Compare July 23, 2025 14:43

Improve file delete performance for expire_snapshots

1be0c5a

- Add ForwardingFileIoFactory so delete specific ExecutorService is not injected in multiple places - Remove direct usage of ForwardingFileIo in favor of a FileIoFactory implementation - Add parallelized deletes in ForwardingFileIo

tbaeg force-pushed the expire-snapshots-parallel branch from d39a0a4 to 1be0c5a Compare July 23, 2025 15:02

Improve file delete performance for expire_snapshots #26230

Are you sure you want to change the base?

Improve file delete performance for expire_snapshots #26230

Conversation

tbaeg commented Jul 17, 2025 • edited by raunaqmorarka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

raunaqmorarka commented Jul 19, 2025

Uh oh!

tbaeg commented Jul 20, 2025

Uh oh!

Uh oh!

Uh oh!

ebyhr left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

findinpath Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findinpath Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

tbaeg Jul 21, 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!

tbaeg Jul 21, 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!

raunaqmorarka Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

tbaeg Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

tbaeg commented Jul 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

raunaqmorarka left a comment

Choose a reason for hiding this comment

Uh oh!

raunaqmorarka Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

tbaeg Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tbaeg commented Jul 17, 2025 •

edited by raunaqmorarka

Loading

findinpath commented Jul 21, 2025 •

edited

Loading

tbaeg Jul 21, 2025 •

edited

Loading

tbaeg Jul 21, 2025 •

edited

Loading

tbaeg commented Jul 22, 2025 •

edited

Loading