Support Iceberg's DROP TABLE for corrupted tables #16674

krvikash · 2023-03-22T16:34:11Z

Description

Follow up of #16651 for corrupted Iceberg Tables

Release notes

(X) Release notes are required, with the following suggested text:

# Iceberg
* Support Iceberg's DROP TABLE for corrupted tables. ({issue}`16674`)

findinpath · 2023-03-23T10:58:59Z

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

+        catch (RuntimeException e) {
+            if (e.getCause() != null) {
+                if (e.getCause() instanceof FileNotFoundException
+                        || e.getCause().getMessage().contains("The specified key does not exist")) {


The check against an error message is not ideal.
For S3, look into TrinoS3FileSystem whether it would be appropriate to throw a FileNotFoundException in such cases

io.trino.plugin.hive.s3.TrinoS3FileSystem.TrinoS3InputStream#read(long, byte[], int, int)

what was the exception previously raised here?

findepi · 2023-03-23T11:59:47Z

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

+
+        // Assert queries fail cleanly
+        assertThatThrownBy(() -> query("TABLE " + tableName))
+                .hasMessageContaining("Failed to open input stream for file:");


are you intentionally using different syntax than in #16651 (5b178ea, Test Delta connector behavior for a corrupted table)?
differences will make it harder to keep the tests aligned, if we want to improve test coverage or just check it's same.

Yes, It was intentional in 1st commit Test Iceberg connector behavior for a corrupted table. The exception thrown is not a TrinoException, that is why I can not use assertQueryFailure and end up using assertThatThrownBy.

However, in the 2nd commit, I replaced assertThatThrownBy with assertQueryFailure.

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

findepi · 2023-03-23T12:03:25Z

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

+        catch (RuntimeException e) {
+            if (e.getCause() != null) {
+                if (e.getCause() instanceof FileNotFoundException
+                        || e.getCause().getMessage().contains("The specified key does not exist")) {


what was the exception previously raised here?

findepi · 2023-03-23T12:05:40Z

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

@@ -356,16 +357,54 @@ public Table loadTable(ConnectorSession session, SchemaTableName table)
    @Override
    public void dropTable(ConnectorSession session, SchemaTableName schemaTableName)
    {
-        BaseTable table = (BaseTable) loadTable(session, schemaTableName);
-        validateTableCanBeDropped(table);
+        com.amazonaws.services.glue.model.Table metastoreTable;


Why do we need any changes to the Drop table logic in a catalog implementation?

perhaps, dropping corrupted table could be done separately, as an unregister_table + delete table directory.
if the table is corrupted, we probably should not expect catalog do anything smarter than that

cc @alexjo2144 @findinpath @electrum thoughts?

unregister_table + delete table directory.

I like this approach. I was concerned when data files of the Iceberg table may be located in different locations, But when there is a corrupted table (metadata file is missing) then we won't able to perform dropTableData because we won't able to load the table.

So I think it is fine to use unregister_table + delete table directory.. But we have to get the table location from metastore and for that, we have to approach the respective catalog to provide the table location. what if we introduce a new method (dropCorruptedTable, forceDrop ...) in the catalog to drop corrupted tables? thoughts?

// Use the Iceberg routine for dropping the table data because the data files // of the Iceberg table may be located in different locations dropTableData(table.io(), table.operations().current());

perhaps, dropping corrupted table could be done separately, as an unregister_table + delete table directory.

Do we have the certainty in such cases that the directory can be safely removed or should this operation be left to the system administrator?
IMO we are speaking here about a corner case which should probably be handled in a best effort manner.

I'd advocate to simply unregister the table.

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

...java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java

...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java

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

findinpath · 2023-03-24T10:56:57Z

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

@@ -356,16 +357,54 @@ public Table loadTable(ConnectorSession session, SchemaTableName table)
    @Override
    public void dropTable(ConnectorSession session, SchemaTableName schemaTableName)
    {
-        BaseTable table = (BaseTable) loadTable(session, schemaTableName);
-        validateTableCanBeDropped(table);
+        com.amazonaws.services.glue.model.Table metastoreTable;


perhaps, dropping corrupted table could be done separately, as an unregister_table + delete table directory.

Do we have the certainty in such cases that the directory can be safely removed or should this operation be left to the system administrator?
IMO we are speaking here about a corner case which should probably be handled in a best effort manner.

I'd advocate to simply unregister the table.

krvikash · 2023-03-27T11:58:33Z

Thanks, @findepi | @findinpath for the reviews. Addressed comments.

krvikash · 2023-03-28T05:38:55Z

Fixed CI failure

findinpath · 2023-03-28T18:49:31Z

Tests are red

023-03-28T10:37:36.1282148Z 2023-03-28T04:37:35.895-0600	ERROR	http-worker-659	org.apache.iceberg.rest.RESTCatalogServlet	Error processing REST request
2023-03-28T10:37:36.1283043Z org.apache.iceberg.exceptions.RESTException: Unhandled error: ErrorResponse(code=404, type=NoSuchTableException, message=Table does not exist: tpch.test_drop_table_with_missing_snapshot_file_kljh1kx0xl)
2023-03-28T10:37:36.1283886Z org.apache.iceberg.exceptions.NoSuchTableException: Table does not exist: tpch.test_drop_table_with_missing_snapshot_file_kljh1kx0xl
2023-03-28T10:37:36.1284622Z 	at org.apache.iceberg.BaseMetastoreCatalog.loadTable(BaseMetastoreCatalog.java:50)
2023-03-28T10:37:36.1285554Z 	at org.apache.iceberg.rest.CatalogHandlers.loadTable(CatalogHandlers.java:240)
2023-03-28T10:37:36.1286219Z 	at org.apache.iceberg.rest.RESTCatalogAdapter.handleRequest(RESTCatalogAdapter.java:336)
2023-03-28T10:37:36.1286891Z 	at org.apache.iceberg.rest.RESTCatalogAdapter.execute(RESTCatalogAdapter.java:384)
2023-03-28T10:37:36.1287535Z 	at org.apache.iceberg.rest.RESTCatalogServlet.execute(RESTCatalogServlet.java:100)
2023-03-28T10:37:36.1288150Z 	at org.apache.iceberg.rest.RESTCatalogServlet.doGet(RESTCatalogServlet.java:66)
2023-03-28T10:37:36.1288707Z 	at javax.servlet.http.HttpServlet.service(HttpServlet.java:503)
2023-03-28T10:37:36.1289213Z 	at javax.servlet.http.HttpServlet.service(HttpServlet.java:590)

2023-03-28T10:44:47.9500066Z 2023-03-28T04:44:47.907-0600	ERROR	pool-3-thread-1	io.trino.testng.services.ProgressLoggingListener	[TEST FAILURE] io.trino.plugin.iceberg.TestIcebergMinioOrcConnectorSmokeTest.testDropTableWithMissingSnapshotFile; (took: 0.9 seconds)
2023-03-28T10:44:47.9501766Z io.trino.testing.QueryFailedException: Failed to get status for file: s3://test-iceberg-minio-smoke-test-vqhe3hnf6z/tpch_orc/test_drop_table_with_missing_snapshot_file_a05pfyc9tt-8c2da560b5424ec1bb46ec83f3cb44bf/metadata/snap-6691070755498700963-1-bd1b58ee-204d-4044-80bd-a02170234d30.avro
2023-03-28T10:44:47.9508936Z 	at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:122)
2023-03-28T10:44:47.9509565Z 	at io.trino.testing.DistributedQueryRunner.executeWithQueryId(DistributedQueryRunner.java:494)
2023-03-28T10:44:47.9510306Z 	at io.trino.testing.QueryAssertions.assertDistributedUpdate(QueryAssertions.java:107)
2023-03-28T10:44:47.9510869Z 	at io.trino.testing.QueryAssertions.assertUpdate(QueryAssertions.java:63)
2023-03-28T10:44:47.9511441Z 	at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:401)
2023-03-28T10:44:47.9512141Z 	at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:396)
2023-03-28T10:44:47.9512961Z 	at io.trino.plugin.iceberg.BaseIcebergConnectorSmokeTest.testDropTableWithMissingSnapshotFile(BaseIcebergConnectorSmokeTest.java:536)
2023-03-28T10:44:47.9513708Z 	at java.base/jdk.int

krvikash · 2023-03-29T05:57:26Z

Fixed CI failure.

krvikash · 2023-04-06T17:04:31Z

Addressed comments.

krvikash · 2023-04-06T17:05:08Z

rebased with master.

krvikash · 2023-04-07T07:03:28Z

Fixed CI failure.

…SmokeTest

krvikash · 2023-04-12T11:02:39Z

rebased with master and resolved conflicts

...java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java

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

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

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java

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

findepi · 2023-04-12T11:54:44Z

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

+        String tableLocation = getTableLocation(tableName);
+
+        // Delete current metadata file
+        trinoFileSystem.deleteFile(metadataLocation);


Can this affect concurrently executing tests (in case they query information_schema.columns)?

i hope not (i'd be a product problem, not a test issue), but let's keep an eye

I think that in such cases the columns of the table don't get taken into account while querying information_schema.columns

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

Lines 642 to 646 in 0c99af4

catch (RuntimeException e) {

// Table can be being removed and this may cause all sorts of exceptions. Log, because we're catching broadly.

log.warn(e, "Failed to access metadata of table %s during streaming table columns for %s", tableName, prefix);

return Stream.empty();

}

krvikash · 2023-04-13T09:31:19Z

Addressed comments.

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

Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>

krvikash · 2023-04-17T11:22:55Z

Addressed comments.

findinpath · 2023-04-20T15:37:37Z

...in/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorTest.java

+    @Override
+    public void testCorruptedTableLocation()
+    {
+        throw new SkipException("Skipping test, This test override will be removed in next commit");


nit It's not clear from the exception why the skipping is being done - even if it is temporary.

findinpath · 2023-04-20T16:16:26Z

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

@@ -6708,47 +6715,162 @@ public void testAlterTableWithUnsupportedProperties()
        assertUpdate("DROP TABLE " + tableName);
    }

+    @Test
+    public void testDropTableWithMissingMetadataFile()


we have these tests on both the Iceberg BCT as well as on Iceberg BCST

why?

BCT should have no less coverage than BCST

we need BCST so that all Catalog impls are exercised.

findinpath · 2023-04-20T16:20:40Z

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

+    }
+
+    @Test
+    public void testDropTableWithMissingSnapshotFile()


We should also make sure that we don't delete on DROP more than the content associated to the corrupted table.

I'm thinking that a test where two tables exist before the drop:

one table is ok

one table is corrupt

When dropping the corrupt table, the OK table should still be present in the metastore and the amount of files in the storage (within the test schema) should decrease with only the number of files corresponding to the corrupted table.

having the test as part of BCT / BCST provides that. there are other test tables (like nation) which don't get deleted.

findinpath

LGTM % comments

krvikash · 2023-04-24T15:24:40Z

Thank you all for reviewing the PR 😊

cla-bot bot added the cla-signed label Mar 22, 2023

krvikash requested review from findinpath, findepi, ebyhr and alexjo2144 March 22, 2023 16:35

krvikash self-assigned this Mar 22, 2023

github-actions bot added iceberg Iceberg connector tests:hive labels Mar 22, 2023

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch 3 times, most recently from 2e44d09 to bf58ccf Compare March 23, 2023 09:51

findinpath reviewed Mar 23, 2023

View reviewed changes

findepi reviewed Mar 23, 2023

View reviewed changes

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from bf58ccf to b9afd7d Compare March 24, 2023 09:11

findinpath reviewed Mar 24, 2023

View reviewed changes

krvikash mentioned this pull request Mar 25, 2023

Support drop iceberg table when metadata or snapshot is missing and refactor code for delta-lake #15065

Closed

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch 2 times, most recently from 6f84d4e to a55225d Compare March 27, 2023 11:49

github-actions bot added delta-lake Delta Lake connector hive Hive connector labels Mar 27, 2023

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from a55225d to 6b746cd Compare March 28, 2023 05:38

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from 6b746cd to 4b5a180 Compare March 28, 2023 05:41

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from 4b5a180 to b521efa Compare March 28, 2023 20:46

krvikash requested review from findepi and findinpath March 29, 2023 05:57

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch 2 times, most recently from 6b2ff5a to 0acb814 Compare April 6, 2023 16:57

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from 0acb814 to 022c934 Compare April 6, 2023 17:04

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from 3abd742 to b7faab2 Compare April 7, 2023 07:03

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from b7faab2 to 907a312 Compare April 7, 2023 12:58

Implement getMetadataLocation in TestIcebergTrinoRestCatalogConnector…

7ef84b4

…SmokeTest

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from 907a312 to 49da147 Compare April 12, 2023 11:02

findepi approved these changes Apr 12, 2023

View reviewed changes

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch 2 times, most recently from 09a5673 to 5710f7d Compare April 13, 2023 09:30

findepi reviewed Apr 13, 2023

View reviewed changes

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

krvikash and others added 2 commits April 17, 2023 15:15

Test Iceberg connector behavior for a corrupted table

a09fbde

Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>

Support Iceberg's DROP TABLE for corrupted tables

ea7d71a

krvikash force-pushed the support-drop-corrupted-iceberg-table-alternative-2 branch from 5710f7d to ea7d71a Compare April 17, 2023 11:21

findinpath reviewed Apr 20, 2023

View reviewed changes

findepi merged commit f5084be into trinodb:master Apr 24, 2023

krvikash deleted the support-drop-corrupted-iceberg-table-alternative-2 branch April 24, 2023 15:24

github-actions bot added this to the 415 milestone Apr 24, 2023

colebow mentioned this pull request Apr 26, 2023

Add Trino 415 release notes #17234

Merged

emmanuel099 mentioned this pull request Jan 12, 2024

Add support for dropping corrupted tables to Iceberg Nessie Catalog #20360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Iceberg's DROP TABLE for corrupted tables #16674

Support Iceberg's DROP TABLE for corrupted tables #16674

krvikash commented Mar 22, 2023 •

edited

Loading

findinpath Mar 23, 2023

findepi Mar 23, 2023

findepi Mar 23, 2023

krvikash Mar 23, 2023

findepi Mar 23, 2023

findepi Mar 23, 2023

krvikash Mar 24, 2023 •

edited

Loading

findinpath Mar 24, 2023

findinpath Mar 24, 2023

krvikash commented Mar 27, 2023

krvikash commented Mar 28, 2023

findinpath commented Mar 28, 2023

krvikash commented Mar 29, 2023

krvikash commented Apr 6, 2023

krvikash commented Apr 6, 2023

krvikash commented Apr 7, 2023

krvikash commented Apr 12, 2023

findepi Apr 12, 2023

findinpath Apr 20, 2023

krvikash commented Apr 13, 2023

krvikash commented Apr 17, 2023

findinpath Apr 20, 2023

findinpath Apr 20, 2023

findepi Apr 24, 2023

findinpath Apr 20, 2023

findepi Apr 24, 2023

findinpath left a comment

krvikash commented Apr 24, 2023

	catch (RuntimeException e) {
	// Table can be being removed and this may cause all sorts of exceptions. Log, because we're catching broadly.
	log.warn(e, "Failed to access metadata of table %s during streaming table columns for %s", tableName, prefix);
	return Stream.empty();
	}

Support Iceberg's DROP TABLE for corrupted tables #16674

Support Iceberg's DROP TABLE for corrupted tables #16674

Conversation

krvikash commented Mar 22, 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 Mar 24, 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 Mar 27, 2023

krvikash commented Mar 28, 2023

findinpath commented Mar 28, 2023

krvikash commented Mar 29, 2023

krvikash commented Apr 6, 2023

krvikash commented Apr 6, 2023

krvikash commented Apr 7, 2023

krvikash commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krvikash commented Apr 13, 2023

krvikash commented Apr 17, 2023

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 left a comment

Choose a reason for hiding this comment

krvikash commented Apr 24, 2023

krvikash commented Mar 22, 2023 •

edited

Loading

krvikash Mar 24, 2023 •

edited

Loading