Ignore TableNotFoundException on retries when dropping the table

trinodb · Jan 10, 2024 · a6eb444 · a6eb444
1 parent 5113d9b
commit a6eb444
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 2 deletions.
diff --git a/...n/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java b/...n/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java
@@ -103,6 +103,7 @@
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
 import java.util.stream.Stream;
 
@@ -1062,13 +1063,27 @@ public void createTable(Table table)
     @Override
     public void dropTable(String databaseName, String tableName, boolean deleteData)
     {
+        AtomicInteger attemptCount = new AtomicInteger();
         try {
             retry()
                     .stopOn(NoSuchObjectException.class)
                     .stopOnIllegalExceptions()
                     .run("dropTable", stats.getDropTable().wrap(() -> {
                         try (ThriftMetastoreClient client = createMetastoreClient()) {
-                            Table table = client.getTable(databaseName, tableName);
+                            attemptCount.incrementAndGet();
+                            Table table;
+                            try {
+                                table = client.getTable(databaseName, tableName);
+                            }
+                            catch (NoSuchObjectException e) {
+                                if (attemptCount.get() == 1) {
+                                    // Throw exception only on first attempt.
+                                    throw e;
+                                }
+                                // If table is not found on consecutive attempts it was probably dropped on first attempt and timeout occurred.
+                                // Exception in such case can be safely ignored and dropping table is finished.
+                                return null;
+                            }
                             client.dropTable(databaseName, tableName, deleteData);
                             String tableLocation = table.getSd().getLocation();
                             if (deleteFilesOnDrop && deleteData && isManagedTable(table) && !isNullOrEmpty(tableLocation)) {

diff --git a/...in/trino-hive/src/test/java/io/trino/plugin/hive/metastore/AbstractTestHiveMetastore.java b/...in/trino-hive/src/test/java/io/trino/plugin/hive/metastore/AbstractTestHiveMetastore.java
@@ -15,6 +15,7 @@
 
 import io.trino.plugin.hive.SchemaAlreadyExistsException;
 import io.trino.plugin.hive.TableAlreadyExistsException;
+import io.trino.spi.connector.TableNotFoundException;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
 import org.junit.jupiter.api.parallel.Execution;
@@ -30,6 +31,7 @@
 import static io.trino.plugin.hive.TableType.EXTERNAL_TABLE;
 import static io.trino.plugin.hive.metastore.PrincipalPrivileges.NO_PRIVILEGES;
 import static io.trino.plugin.hive.metastore.StorageFormat.fromHiveStorageFormat;
+import static io.trino.testing.TestingNames.randomNameSuffix;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
 import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
@@ -102,4 +104,20 @@ void testCreateTable()
         metastore.dropTable(databaseName, tableName, false);
         metastore.dropDatabase(databaseName, false);
     }
+
+    @Test
+    public void testDropNotExistingTable()
+    {
+        String databaseName = "test_database_" + randomNameSuffix();
+        Database.Builder database = Database.builder()
+                .setDatabaseName(databaseName)
+                .setOwnerName(Optional.empty())
+                .setOwnerType(Optional.empty());
+        getMetastore().createDatabase(database.build());
+
+        assertThatThrownBy(() -> getMetastore().dropTable(databaseName, "not_existing", false))
+                .isInstanceOf(TableNotFoundException.class);
+
+        getMetastore().dropDatabase(databaseName, false);
+    }
 }
diff --git a/...in/trino-hive/src/test/java/io/trino/plugin/hive/metastore/TestBridgingHiveMetastore.java b/...in/trino-hive/src/test/java/io/trino/plugin/hive/metastore/TestBridgingHiveMetastore.java
@@ -37,6 +37,7 @@
 import static io.trino.plugin.hive.metastore.PrincipalPrivileges.NO_PRIVILEGES;
 import static io.trino.plugin.hive.metastore.StorageFormat.fromHiveStorageFormat;
 import static io.trino.testing.TestingNames.randomNameSuffix;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
 import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
@@ -61,7 +62,7 @@ final class TestBridgingHiveMetastore
             catch (InvocationTargetException e) {
                 throw e.getCause();
             }
-            if (method.getName().equals("createDatabase") || method.getName().equals("createTable")) {
+            if (method.getName().equals("createDatabase") || method.getName().equals("createTable") || method.getName().equals("dropTable")) {
                 throw new RuntimeException("Test-simulated Hive Metastore timeout exception");
             }
             return result;
@@ -127,4 +128,31 @@ public void testCreateTableWithRetries()
         getMetastore().dropTable(databaseName, tableName, false);
         getMetastore().dropDatabase(databaseName, false);
     }
+
+    @Test
+    public void testDropTableWithRetries()
+    {
+        String databaseName = "test_database_" + randomNameSuffix();
+        Database.Builder database = Database.builder()
+                .setDatabaseName(databaseName)
+                .setOwnerName(Optional.empty())
+                .setOwnerType(Optional.empty());
+        getMetastore().createDatabase(database.build());
+
+        String tableName = "test_table" + randomNameSuffix();
+        Table.Builder table = Table.builder()
+                .setDatabaseName(databaseName)
+                .setTableName(tableName)
+                .setTableType(EXTERNAL_TABLE.name())
+                .setOwner(Optional.empty());
+        table.getStorageBuilder()
+                .setStorageFormat(fromHiveStorageFormat(PARQUET));
+        getMetastore().createTable(table.build(), NO_PRIVILEGES);
+
+        assertThat(getMetastore().getTable(databaseName, tableName)).isPresent();
+        getMetastore().dropTable(databaseName, tableName, false);
+        assertThat(getMetastore().getTable(databaseName, tableName)).isEmpty();
+
+        getMetastore().dropDatabase(databaseName, false);
+    }
 }