Improve performance of drop table in iceberg connector #15981

krvikash · 2023-02-06T04:15:32Z

Description

While working on Iceberg small files benchmarking. I noticed that the drop table is very slow when the number of files is too large for a table. The main reason for slowness is dropTableData, which deletes the files concurrently when the fileIO is not an instance of SupportsBulkOperations. we can delete files in bulk if fileIO is an instance of SupportsBulkOperations. So implements SupportsBulkOperations instead of FileIO in ForwardingFileIo.java.

I tested it on my local machine. A table with glue metastore containing data files approx 10K took ~26 minutes without this change whereas with this change it only took ~20 seconds.

Release notes

( ) This is not user-visible or 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:

# Section
* Improve performance of drop table in iceberg connector. ({issue}`15981`)

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIO.java

ebyhr · 2023-02-06T05:17:36Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIO.java

+            fileSystem.deleteFiles(pathList.build());
+        }
+        catch (IOException e) {
+            throw new RuntimeException(e);


Throwing RuntimeException is different from the interface's expectation. It would be nice to throw BulkDeletionFailureException or leave a comment.

BulkDeletionFailureException requires the number of files that failed to delete. Currently fileSystem.deleteFiles does not give such information. I will prepare a PR for the same.

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIO.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

findepi · 2023-02-06T09:07:17Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIo.java

+public class BulkOperationsFileIo
+        extends ForwardingFileIo


I would implement this directly in ForwardingFileIo, not as a subclass.

(that would probably also satisfy https://github.com/trinodb/trino/pull/15981/files#r1097092879, right?)

cc @electrum

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIo.java

findepi · 2023-02-06T09:08:44Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIo.java

+            fileSystem.deleteFiles(filesToDelete);
+        }
+        catch (IOException e) {
+            // TODO find out the number of files that failed to delete and throw BulkDeletionFailureException


what is org.apache.iceberg.io.BulkDeletionFailureException#numberFailedObjects needed for?

if we don't need it, we can skip having a TODO

SupportsBulkOperations::deleteFiles throws BulkDeletionFailureException exception. BulkDeletionFailureException will be constructed by providing an integer value numberFailedObjects which means the number of files that failed to delete.

SupportsBulkOperations::deleteFiles throws BulkDeletionFailureException exception.

I know.
BTW it can also throw any unckeched exception, obviously.

numberFailedObjects which means the number of files that failed to delete.

I figured this from the name :)
How does Iceberg lib use that information?

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIo.java

krvikash · 2023-02-06T13:26:43Z

Addressed comments.

CI is failing due to a flaky test

2023-02-06T12:27:38.4496953Z tests | 2023-02-06 18:12:38 INFO: FAILURE / io.trino.tests.product.kudu.TestKuduConnectoKerberosSmokeTest.kerberosAuthTicketExpiryTest (Groups: profile_specific_tests, kudu) took 3.3 seconds

#14441

electrum · 2023-02-06T20:38:57Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/ForwardingFileIo.java

+
+    @Override
+    public void deleteFiles(Iterable<String> pathsToDelete)
+            throws BulkDeletionFailureException


This isn’t thrown by the method. Should we be using this instead?

we cannot throw BulkDeletionFailureException becauase we cannot fill BulkDeletionFailureException#numberFailedObjects

see #15981 (comment) for more

ebyhr · 2023-02-07T07:49:04Z

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/ForwardingFileIo.java

+            fileSystem.deleteFiles(filesToDelete);
+        }
+        catch (IOException e) {
+            throw new UncheckedIOException("Failed to delete some or all of files: " + filesToDelete, e);


I don't think including all file paths to the error message is a good idea in case of large deletes. It may include 1,000 files.

Hi @ebyhr| @findepi | @electrum , I am proposing a PR #16006 to track the number of files that failed to delete. If #16006 gets approved then we can throw BulkDeletionFailureException over here.

@ebyhr you convinced me, let's not make the message that long. OTOH, it could be problematic if we do not include any paths in the message. It can make identifying problems harder (eg which bucket did we try to access?).
I pushed a change to display only first 5 paths and then ellipsis ....

cla-bot bot added the cla-signed label Feb 6, 2023

krvikash added release-notes performance labels Feb 6, 2023

krvikash marked this pull request as ready for review February 6, 2023 04:25

krvikash self-assigned this Feb 6, 2023

ebyhr reviewed Feb 6, 2023

View reviewed changes

krvikash force-pushed the delete-table-s3-bulk branch from d5565c6 to 1f921c2 Compare February 6, 2023 06:10

krvikash removed the release-notes label Feb 6, 2023

findinpath requested a review from homar February 6, 2023 08:51

findepi reviewed Feb 6, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java Outdated Show resolved Hide resolved

findepi reviewed Feb 6, 2023

View reviewed changes

homar reviewed Feb 6, 2023

View reviewed changes

lib/trino-filesystem/src/main/java/io/trino/filesystem/fileio/BulkOperationsFileIo.java Outdated Show resolved Hide resolved

krvikash force-pushed the delete-table-s3-bulk branch 2 times, most recently from 17b4302 to 30eff29 Compare February 6, 2023 10:06

electrum reviewed Feb 6, 2023

View reviewed changes

findepi approved these changes Feb 6, 2023

View reviewed changes

ebyhr approved these changes Feb 7, 2023

View reviewed changes

krvikash mentioned this pull request Feb 7, 2023

Enhance TrinoFileSystem#deleteFiles with files failure count #16006

Closed

findepi force-pushed the delete-table-s3-bulk branch from b986679 to 659a3b0 Compare February 7, 2023 09:25

Improve performance of drop table in iceberg connector

3310ec6

findepi force-pushed the delete-table-s3-bulk branch from 659a3b0 to 3310ec6 Compare February 7, 2023 09:29

findepi merged commit e41c3a7 into trinodb:master Feb 7, 2023

findepi mentioned this pull request Feb 7, 2023

Release notes for 407 #15854

Closed

krvikash deleted the delete-table-s3-bulk branch February 7, 2023 17:21

github-actions bot added this to the 407 milestone Feb 7, 2023

colebow mentioned this pull request Feb 10, 2023

Add Trino 407 release notes #15919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of drop table in iceberg connector #15981

Improve performance of drop table in iceberg connector #15981

krvikash commented Feb 6, 2023 •

edited

Loading

ebyhr Feb 6, 2023

krvikash Feb 6, 2023

findepi Feb 6, 2023

findepi Feb 6, 2023

krvikash Feb 6, 2023

findepi Feb 6, 2023

krvikash commented Feb 6, 2023

electrum Feb 6, 2023

findepi Feb 6, 2023

ebyhr Feb 7, 2023 •

edited

Loading

krvikash Feb 7, 2023

findepi Feb 7, 2023

Improve performance of drop table in iceberg connector #15981

Improve performance of drop table in iceberg connector #15981

Conversation

krvikash commented Feb 6, 2023 • edited Loading

Description

Release notes

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

krvikash commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krvikash commented Feb 6, 2023 •

edited

Loading

ebyhr Feb 7, 2023 •

edited

Loading