Support delete corrupted Iceberg table by yabola · Pull Request #5510 · apache/iceberg

yabola · 2022-08-12T07:51:22Z

When the metadata file doesn't exist any more, drop table operation will fail.
This PR aims to support deleting the table even if the table was corrupted.

yabola · 2022-08-12T08:11:02Z

+        lastMetadata = null;
+      }
+    } catch (Exception e) {
+      if (e instanceof NotFoundException) {


@szehon-ho cc~ does this need to make a separate method ? if separate , it may load table twice.

ajantha-bhat · 2022-08-12T10:59:22Z

+      } else {
+        lastMetadata = null;
+      }
+    } catch (Exception e) {


Can directly just catch NotFoundException? Instead of catching all the exceptions and throwing again if it is not a NotFoundException?

Yes, I'm sorry, my compiler had a problem before, it has been modified

ajantha-bhat · 2022-08-12T11:03:32Z

+    } catch (Exception e) {
+      if (e instanceof NotFoundException) {
+        LOG.warn("The metadata file doesn't exist any more. Continue to drop the table.", e);
+        lastMetadata = null;


I am not sure if this needs to be supported.

See the line 199,
If the lastMetadata is null, table files are never deleted even with purge = true.

if (purge && lastMetadata != null) { CatalogUtil.dropTableData(ops.io(), lastMetadata); }

ajantha-bhat · 2022-08-12T11:04:39Z

+    catalog.createTable(identifier, tableSchema);
+    TableOperations ops = catalog.newTableOps(identifier);
+
+    Assert.assertTrue(catalog.dropTable(identifier, true));


nit: I feel no need to test line 473 to 480

I agree, let's remove L473 - 478 and just focus on the actual case where we don't have a metadata file for the given table

ajantha-bhat

When the NotFoundException is thrown,
we can call drop table again with purge = false which will succeed.

So, I think these changes are not required. Let us wait for others' opinions.

Also, this PR handles only one catalog. Instead, manually calling drop table again with purge = false works for all the catalogs.

nastra

the approach LGTM, just a few nits to address

nastra · 2022-08-19T09:06:00Z

+        lastMetadata = null;
+      }
+    } catch (NotFoundException e) {
+      LOG.warn("The metadata file doesn't exist any more. Continue to drop the table.", e);


Suggested change

LOG.warn("The metadata file doesn't exist any more. Continue to drop the table.", e);

LOG.warn("The metadata file doesn't exist anymore. Continuing to drop the table.", e);

nastra · 2022-08-19T09:08:28Z

+        NoSuchTableException.class,
+        () -> catalog.loadTable(identifier));
+
+    // delete corrupted table


maybe just // delete metadata file

nastra · 2022-08-19T09:09:30Z

+    Assert.assertTrue(catalog.dropTable(identifier, true));
+    AssertHelpers.assertThrows(
+        "Should fail to load table after dropping it",
+        NoSuchTableException.class,


nit: let's use

Assertions.assertThatThrownBy(() -> catalog.loadTable(identifier)).isInstanceOf(NoSuchTableException.class).hasMessage("xx")

nastra · 2022-08-19T09:10:26Z

+    catalog.createTable(identifier, tableSchema);
+    TableOperations ops = catalog.newTableOps(identifier);
+
+    Assert.assertTrue(catalog.dropTable(identifier, true));


I agree, let's remove L473 - 478 and just focus on the actual case where we don't have a metadata file for the given table

nastra · 2022-08-19T09:10:50Z

  }

+  @Test
+  public void testDropTable() throws TException {


Suggested change

public void testDropTable() throws TException {

public void testDropTableWithoutMetadataFile() throws TException {

yabola · 2022-08-19T10:11:20Z

@nastra I had addressed comments. Thanks for review.

nastra

this LGTM, @pvary or @rdblue could one of you please review this as well?
This should fix #5584

gaborkaszab · 2022-08-19T10:50:53Z

We also bumped into this when testing through Apache Impala. This fix could be useful for us as well.

szehon-ho · 2022-08-20T07:19:29Z

+      if (purge && ops.current() != null) {
+        lastMetadata = ops.current();
+      } else {
+        lastMetadata = null;


Style nit: Wondering, while we are in this code, it may be a bit more concise to initialize lastMetadata = null, then we can remove two null assignments in the else/catch blocks? Not sure what people think, though its not a big in any case.

Might be a personal preference, but I also prefer initialising to null and then save an else branch of an if.

It is better to initialize to null from the concise code, but I think keeping null is easier to understand in terms of readability.
I think It depends on whether the lastMetadata = null has a significant effect on the method . If it is not that important, we can initialise it as null.

lastMetadata = null seems not that important for dropTable. So I initialise it as null.

rdblue · 2022-08-20T21:38:54Z

+        lastMetadata = ops.current();
+      }
+    } catch (NotFoundException e) {
+      LOG.warn("The metadata file doesn't exist anymore. Continuing to drop the table.", e);


In Iceberg, log messages should be specific statements and should not speculate about underlying causes. In this case, the operations failed to load table metadata. That doesn't necessarily mean that it doesn't exist. The error should state that clearly. Failed to load table metadata for table: %s, continuing drop without purge

I have updated log message.

pvary · 2022-08-21T07:14:09Z


+  @Test
+  public void testDropTableWithoutMetadataFile() {
+    Namespace namespace = Namespace.of("dbname_drop");


Instead of creating a new namespace in the HiveCatalog tests we are usually using a static db:

TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");

I have updated it.

pvary · 2022-08-21T07:19:42Z

Quick question: do we need this only for HiveCatalog? Do we need this for other catalogs as well? (Maybe a different PR, but I think we would expect the same behavior for every Catalog implementation, or at least a clean documentation for the differences)

yabola · 2022-08-21T12:19:23Z

@pvary I can complete other catalogs in this PR if need.

rdblue · 2022-08-21T16:39:20Z

+      }
+    } catch (NotFoundException e) {
+      LOG.warn(
+          "Failed to load table metadata for table: {}, continuing drop without purge.",


Please removing ending punctuation. There's no need for it.

rdblue · 2022-08-21T16:40:43Z

I agree with @pvary. It would be better to add this test to CatalogTests and ensure that all the catalogs behave this way.

singhpk234 · 2022-08-22T14:39:32Z

+1, seems like same issue is reported for GlueCatalog : #5604, we should have similar handling for GlueCatalog as well, having tests for all catalogs could have helped detecting :) !!

yabola · 2022-08-22T15:50:56Z

@singhpk234 I will complete all catalogs in this PR~

yabola · 2022-08-24T12:38:52Z

I have implemented HiveCatalog, GlueCatalog,JdbcCatalog, DynamoDbCatalog.
BaseSessionCatalog , NessieCatalog, RESTCatalog don't have purge table.
EcsCatalog, HadoopCatalog purge logic (fail if metadata doesn't exist) was different from HiveCatalog. So I am not sure if it need to change.

yabola · 2022-08-25T02:59:02Z

Also note that, JdbcCatalog, GlueTableOperationsand other will retry 20 times and cost some time when refresh metadata and there is no metadata file. Please take a look at BaseMetastoreTableOperations#refreshFromMetadataLocation.
HiveCatalog will only retry 2 times , please see parameter iceberg.hive.metadata-refresh-max-retries.

yabola · 2022-08-29T16:58:01Z

@rdblue @szehon-ho @pvary If you have time, please take a look. And I have one question as above. If it is not suitable, can I only change HiveCatalog ?

szehon-ho · 2022-08-29T17:47:47Z

@yabola I'm ok with some retry, not sure if its worth the additional risk we introduce to turn it off.

It does seem 20 can be a bit excessive, wonder whether we can turn that down or it is by design.. maybe others will have an idea like @singhpk234 for GlueCatalog?

But one concern here, does it have an impact on how long the test takes to run, and can we turn it down at least for this test?

singhpk234 · 2022-08-29T20:17:32Z

It does seem 20 can be a bit excessive, wonder whether we can turn that down or it is by design.. maybe others will have an idea like @singhpk234 for GlueCatalog

20 seems to be quite excessive IMHO as well, going by the calculations mentioned in the Hive PR to reduce retries it can lead to hang > 10 min. Unfortunately I don't have much context into what the motivation was to keep it to 20 from the beginning ... The closest I could track it down was this commit 9349a25 from 2018, unfortunately there is not a pr associated with it.

But definitely it makes sense to reduce it to may be 3, and make it configurable like how we did for hive.

yabola · 2022-09-01T01:07:34Z

@szehon-ho My newly added testDropTableWithoutMetadataFile method will take 100s in TestJdbcCatalog, the original Test class will not have this test path, so the time remains unchanged. Sorry that I didn't find a way to avoid retry 20 time for JDBC (20 times is a hard code). So should I delete my new added test method for TestJdbcCatalog or any better idea?

szehon-ho · 2022-09-01T01:36:27Z

Ugh, yea I feel like it would be nice to have this configurable, not just the test but in general, but I feel it may be another pr. If @rdblue is ok to merge, I'm ok.

About the config, it seems like today the way to config things in TableOperations is table property, which could be inherited from CatalogProperties.

gaborkaszab · 2022-09-01T14:52:25Z

@szehon-ho @yabola I created a draft PR to make the number of metadata load retries configurable: #5688
So far I checked JdbcCatalog only but will cover other affected catalogs.

rdblue · 2022-09-01T19:48:15Z

Thanks for taking a look at the metadata retries, @gaborkaszab!

yabola · 2022-09-02T00:54:23Z

@ajantha-bhat @pvary @rdblue @szehon-ho @nastra @singhpk234 @gaborkaszab Thank you all for careful review

github-actions Bot added the hive label Aug 12, 2022

yabola changed the title ~~Support delete corrupted Iceberg table~~ Support delete corrupted Iceberg table in HiveCatalog Aug 12, 2022

yabola changed the title ~~Support delete corrupted Iceberg table in HiveCatalog~~ Support delete corrupted Iceberg table Aug 12, 2022

yabola force-pushed the drop branch from b6f4146 to 19586f9 Compare August 12, 2022 07:58

yabola commented Aug 12, 2022

View reviewed changes

ajantha-bhat reviewed Aug 12, 2022

View reviewed changes

nastra mentioned this pull request Aug 19, 2022

Table remains in HMS when using HiveCatalog.dropTable() after dropping the files of the table from HDFS #5584

Closed

nastra reviewed Aug 19, 2022

View reviewed changes

nastra added this to the Iceberg 0.14.1 Release milestone Aug 19, 2022

nastra approved these changes Aug 19, 2022

View reviewed changes

szehon-ho approved these changes Aug 20, 2022

View reviewed changes

rdblue reviewed Aug 20, 2022

View reviewed changes

yabola force-pushed the drop branch from 337000f to fccce69 Compare August 21, 2022 01:40

pvary reviewed Aug 21, 2022

View reviewed changes

rdblue reviewed Aug 21, 2022

View reviewed changes

rdblue linked an issue Aug 22, 2022 that may be closed by this pull request

Delete table entry from Glue data catalog if corresponding table metadata files are deleted in s3 #5604

Closed

github-actions Bot added AWS core labels Aug 24, 2022

yabola commented Aug 24, 2022

View reviewed changes

Comment thread aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java

yabola force-pushed the drop branch from 844a18d to 3f8df1d Compare August 24, 2022 16:00

Support delete table without metadata file

3f6ebe5

yabola force-pushed the drop branch from 3f8df1d to 3f6ebe5 Compare August 25, 2022 02:53

yabola requested review from rdblue and szehon-ho and removed request for rdblue and szehon-ho August 26, 2022 02:19

rdblue approved these changes Aug 31, 2022

View reviewed changes

rdblue merged commit 26cdc7a into apache:master Sep 1, 2022

rdblue pushed a commit to rdblue/iceberg that referenced this pull request Sep 2, 2022

Core: Support deleting tables without metadata files (apache#5510)

34ee5ce

rdblue pushed a commit to rdblue/iceberg that referenced this pull request Sep 2, 2022

Core: Support deleting tables without metadata files (apache#5510)

a44c0fe

rdblue pushed a commit to rdblue/iceberg that referenced this pull request Sep 2, 2022

Core: Support deleting tables without metadata files (apache#5510)

d8cbcbf

rdblue pushed a commit to rdblue/iceberg that referenced this pull request Sep 2, 2022

Core: Support deleting tables without metadata files (apache#5510)

8fc61f6

rdblue pushed a commit that referenced this pull request Sep 3, 2022

Core: Support deleting tables without metadata files (#5510)

950e6c0

nastra mentioned this pull request Nov 7, 2022

if i lost a metadata file, how to recover #6136

Closed

krvikash mentioned this pull request Nov 14, 2022

Support drop iceberg table when metadata file is missing trinodb/trino#15007

Closed

sunchao pushed a commit to sunchao/iceberg that referenced this pull request May 10, 2023

Core: Support deleting tables without metadata files (apache#5510)

844085c

singhpk234 mentioned this pull request Nov 21, 2023

GlueTableOperations retries on Access Denied exceptions from S3, and does not support configuration of exception retry logic #9124

Closed

singhpk234 mentioned this pull request Feb 24, 2025

More sensible BasePolarisCatalog retry behavior apache/polaris#1046

Merged

zhongyujiang added a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

Core: Support deleting tables without metadata files (apache#5510)

6a4d0ab

	LOG.warn("The metadata file doesn't exist any more. Continue to drop the table.", e);
	LOG.warn("The metadata file doesn't exist anymore. Continuing to drop the table.", e);

	public void testDropTable() throws TException {
	public void testDropTableWithoutMetadataFile() throws TException {

Conversation

yabola commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yabola Aug 12, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat left a comment • 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!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yabola commented Aug 19, 2022

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

gaborkaszab commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yabola Aug 20, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvary commented Aug 21, 2022

Uh oh!

yabola commented Aug 21, 2022

Uh oh!

rdblue Aug 21, 2022 • 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!

rdblue commented Aug 21, 2022

Uh oh!

singhpk234 commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yabola commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yabola commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yabola commented Aug 12, 2022 •

edited

Loading

yabola Aug 12, 2022 •

edited

Loading

ajantha-bhat left a comment •

edited

Loading

yabola Aug 20, 2022 •

edited

Loading

rdblue Aug 21, 2022 •

edited

Loading

singhpk234 commented Aug 22, 2022 •

edited

Loading

yabola commented Aug 22, 2022 •

edited

Loading

yabola commented Aug 24, 2022 •

edited

Loading

szehon-ho commented Sep 1, 2022 •

edited

Loading