Cleanup metadata file when `commitNewTable` fails for the Iceberg table #14869

krvikash · 2022-11-02T19:11:36Z

Description

Iceberg connector creates a new metadata file when we do DDL/DML operation. In such case, if the operation gets fails then the metadata file does not get cleanup. Metastore Table Operation can fail in various cases like permission denied, and not having valid credentials. This fix will cleanup the metadata file if gets it is created.

The fix is inspired from https://github.com/apache/iceberg/blob/3cddc9f28c93b9231060ecb6b90e2d524bd5d160/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java#L142

Non-technical explanation

NA

Release notes

(X) 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.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

krvikash · 2022-11-02T19:16:02Z

TODO Add test cases.

krvikash · 2022-11-03T07:05:44Z

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java

+    protected void cleanupMetadata(String metadataLocation)
+    {
+        if (fileIo.newInputFile(metadataLocation).exists()) {
+            fileIo.deleteFile(metadataLocation);


In a local File system, This operation does not delete the parent folder.

.../trino/plugin/iceberg/catalog/file/TestIcebergFileMetastoreTableOperationsCreateFailure.java

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java

homar · 2022-12-29T14:30:35Z

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java

+
+    protected boolean isCommitSuccess(String metadataLocation)
+    {
+        AtomicReference<Boolean> isSuccess = new AtomicReference<>(false);


Why atomic when it runs in one thread?

I took the reference from https://github.com/apache/iceberg/blob/3cddc9f28c93b9231060ecb6b90e2d524bd5d160/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L304

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

findepi · 2023-01-18T13:33:52Z

...berg/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java

+        }
+        catch (SchemaNotFoundException
+               | TableAlreadyExistsException
+               | UnsupportedOperationException e) {


Why UnsupportedOperationException?

I was looking for createTable implementation and the exceptions thrown by them. InMemoryThriftMetastore#createTable throw UnsupportedOperationException.

But now I realized that InMemoryThriftMetastore is implemented for test cases. So UnsupportedOperationException check is needed.

findepi · 2023-01-18T13:34:19Z

...io/trino/plugin/iceberg/catalog/file/TestIcebergFileCreateTableFailureMetadataCleanedUp.java

+
+import java.io.IOException;
+
+@Test(singleThreaded = true)


Why single threaded?

My bad. It was not needed earlier. But now it's needed with the latest change (// testException is a shared mutable state).

findepi · 2023-01-18T13:34:26Z

...rc/test/java/io/trino/plugin/iceberg/catalog/file/BaseIcebergFileCreateTableFailureTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+@Test(singleThreaded = true)


Why single threaded?

findepi · 2023-01-18T13:34:31Z

...trino/plugin/iceberg/catalog/file/TestIcebergFileCreateTableFailureMetadataNotCleanedUp.java

+
+import java.io.IOException;
+
+@Test(singleThreaded = true)


Why single threaded?

...io/trino/plugin/iceberg/catalog/file/TestIcebergFileCreateTableFailureMetadataCleanedUp.java

findepi · 2023-01-18T13:37:49Z

...rc/test/java/io/trino/plugin/iceberg/catalog/file/BaseIcebergFileCreateTableFailureTest.java

+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+@Test(singleThreaded = true)
+public abstract class BaseIcebergFileCreateTableFailureTest


It doesn't need to be a base class with two subclasses.

one option is to just move QueryRunner setup to a test method (or helper of a test method) and have ordinary test class with two test methods

clean

the downside is that you pay query runner setup cost twice

another option is to just have a query runner and a test instance field (eg AtomicReference) which determines what exception to throw

this would be very similar to the code you have

a bit less clean as you have a mutable field

but you pay query runner setup cost once only

this is the option i'd implement

I followed the 2nd option.

findepi · 2023-01-18T13:39:37Z

...rc/test/java/io/trino/plugin/iceberg/catalog/file/BaseIcebergFileCreateTableFailureTest.java

+            throws Exception
+    {
+        this.dataDirectory = Files.createTempDirectory("test_iceberg_create_table_failure");
+        this.metastore = new FileHiveMetastore(


I am not entirely sure we need that test, since file metastore is just a testing utliity.
if we use file metastore as an approximation of HMS, that's also fine, but let's have a code comment explaining that

findinpath · 2023-02-02T11:04:41Z

@krvikash please rebase & resolve the conflicts.

krvikash · 2023-02-02T12:05:22Z

Rebased with master.

krvikash · 2023-02-13T09:33:34Z

Hi @ebyhr, when you get time could you please review this PR?

...rc/test/java/io/trino/plugin/iceberg/catalog/file/TestIcebergFileCreateTableFailureTest.java

...rc/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCreateTableFailureTest.java

krvikash · 2023-02-14T06:34:46Z

Thanks, @ebyhr for the review. I have addressed the comments and updated the PR.

ebyhr · 2023-02-14T06:57:30Z

/test-with-secrets sha=311ae6e98b205d8d9eea761fd75b0b4d1cd375d7

ebyhr · 2023-02-15T05:01:12Z

I pushed a small fix to rename test classes:

TestIcebergFileCreateTableFailureTest → TestIcebergFileMetastoreCreateTableFailure
TestIcebergGlueCreateTableFailureTest → TestIcebergGlueCreateTableFailure

cla-bot bot added the cla-signed label Nov 2, 2022

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from ce68802 to 8106393 Compare November 2, 2022 19:24

krvikash changed the title ~~Fix metadata file cleanup in case of failure~~ Cleanup metadata file when commit fails for the Iceberg table Nov 2, 2022

krvikash force-pushed the fix-iceberg-metadata-cleanup branch 2 times, most recently from 4d110da to d2e44a1 Compare November 3, 2022 06:50

krvikash changed the title ~~Cleanup metadata file when commit fails for the Iceberg table~~ Cleanup metadata file when commitNewTable fails for the Iceberg table Nov 3, 2022

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from d2e44a1 to bb34560 Compare November 3, 2022 06:57

krvikash commented Nov 3, 2022

View reviewed changes

krvikash self-assigned this Nov 7, 2022

krvikash force-pushed the fix-iceberg-metadata-cleanup branch 3 times, most recently from cabcba5 to f1d331c Compare November 17, 2022 08:01

findinpath reviewed Nov 17, 2022

View reviewed changes

.../trino/plugin/iceberg/catalog/file/TestIcebergFileMetastoreTableOperationsCreateFailure.java Outdated Show resolved Hide resolved

findinpath reviewed Nov 17, 2022

View reviewed changes

.../trino/plugin/iceberg/catalog/file/TestIcebergFileMetastoreTableOperationsCreateFailure.java Outdated Show resolved Hide resolved

findinpath reviewed Nov 17, 2022

View reviewed changes

.../trino/plugin/iceberg/catalog/file/TestIcebergFileMetastoreTableOperationsCreateFailure.java Outdated Show resolved Hide resolved

krvikash changed the title ~~Cleanup metadata file when commitNewTable fails for the Iceberg table~~ WIP: Cleanup metadata file when commitNewTable fails for the Iceberg table Dec 22, 2022

krvikash force-pushed the fix-iceberg-metadata-cleanup branch 3 times, most recently from 0150fee to 95036ab Compare December 27, 2022 13:29

krvikash commented Dec 27, 2022

View reviewed changes

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java Outdated Show resolved Hide resolved

krvikash marked this pull request as ready for review December 27, 2022 14:02

krvikash added the no-release-notes This pull request does not require release notes entry label Dec 27, 2022

krvikash changed the title ~~WIP: Cleanup metadata file when commitNewTable fails for the Iceberg table~~ Cleanup metadata file when commitNewTable fails for the Iceberg table Dec 27, 2022

krvikash force-pushed the fix-iceberg-metadata-cleanup branch 3 times, most recently from e90cc3a to c287c6a Compare December 29, 2022 13:26

krvikash requested a review from homar December 29, 2022 14:15

homar reviewed Dec 29, 2022

View reviewed changes

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from c287c6a to 4919a81 Compare December 29, 2022 14:32

krvikash force-pushed the fix-iceberg-metadata-cleanup branch 2 times, most recently from 5e5a1ad to 2faf497 Compare January 18, 2023 12:40

findepi reviewed Jan 18, 2023

View reviewed changes

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

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from 2faf497 to 1fcb7c2 Compare January 18, 2023 13:12

findepi mentioned this pull request Jan 18, 2023

Enable some additional error-prone checks #15757

Merged

findepi reviewed Jan 18, 2023

View reviewed changes

krvikash mentioned this pull request Jan 18, 2023

Use Failsafe to retry instead iceberg's Tasks in iceberg connector #15758

Merged

findepi reviewed Jan 18, 2023

View reviewed changes

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from 1fcb7c2 to 915e512 Compare January 18, 2023 17:16

krvikash mentioned this pull request Jan 19, 2023

Files get created on S3 while creating a iceberg table even if schema does not exist #15779

Closed

findepi mentioned this pull request Jan 19, 2023

Make it explicit that metrics reporter is required apache/iceberg#6474

Merged

findinpath approved these changes Jan 19, 2023

View reviewed changes

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from 915e512 to bdf2fea Compare February 2, 2023 12:05

ebyhr approved these changes Feb 14, 2023

View reviewed changes

krvikash force-pushed the fix-iceberg-metadata-cleanup branch from bdf2fea to 311ae6e Compare February 14, 2023 06:32

ebyhr force-pushed the fix-iceberg-metadata-cleanup branch from 311ae6e to d657166 Compare February 15, 2023 05:00

ebyhr force-pushed the fix-iceberg-metadata-cleanup branch from d657166 to b36e2aa Compare February 15, 2023 05:02

Cleanup metadata file when commitNewTable fails for the Iceberg table

b8dd25d

ebyhr force-pushed the fix-iceberg-metadata-cleanup branch from b36e2aa to b8dd25d Compare February 15, 2023 05:02

ebyhr merged commit 666dc6f into trinodb:master Feb 16, 2023

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

krvikash deleted the fix-iceberg-metadata-cleanup branch February 16, 2023 06:59

colebow mentioned this pull request Feb 16, 2023

Add Trino 407 release notes #15919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup metadata file when `commitNewTable` fails for the Iceberg table #14869

Cleanup metadata file when `commitNewTable` fails for the Iceberg table #14869

krvikash commented Nov 2, 2022 •

edited

Loading

krvikash commented Nov 2, 2022

krvikash Nov 3, 2022

homar Dec 29, 2022

krvikash Jan 4, 2023

findepi Jan 18, 2023

krvikash Jan 18, 2023

findepi Jan 18, 2023

krvikash Jan 18, 2023

findepi Jan 18, 2023

findepi Jan 18, 2023

findepi Jan 18, 2023

krvikash Jan 18, 2023

findepi Jan 18, 2023

findinpath commented Feb 2, 2023

krvikash commented Feb 2, 2023

krvikash commented Feb 13, 2023

krvikash commented Feb 14, 2023

ebyhr commented Feb 14, 2023

ebyhr commented Feb 15, 2023 •

edited

Loading


		import java.io.IOException;

		@Test(singleThreaded = true)


		import java.io.IOException;

		@Test(singleThreaded = true)

Cleanup metadata file when commitNewTable fails for the Iceberg table #14869

Cleanup metadata file when commitNewTable fails for the Iceberg table #14869

Conversation

krvikash commented Nov 2, 2022 • edited Loading

Description

Non-technical explanation

Release notes

krvikash commented Nov 2, 2022

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

Choose a reason for hiding this comment

findinpath commented Feb 2, 2023

krvikash commented Feb 2, 2023

krvikash commented Feb 13, 2023

krvikash commented Feb 14, 2023

ebyhr commented Feb 14, 2023

ebyhr commented Feb 15, 2023 • edited Loading

Cleanup metadata file when `commitNewTable` fails for the Iceberg table #14869

Cleanup metadata file when `commitNewTable` fails for the Iceberg table #14869

krvikash commented Nov 2, 2022 •

edited

Loading

ebyhr commented Feb 15, 2023 •

edited

Loading