Repository: incubator-impala Updated Branches: refs/heads/master 8d4f8d8d9 -> d484d2f68
IMPALA-4357: Fix DROP TABLE to pass analysis if the table fails to load If a table fails to load, eg. because it was deleted externally from Kudu, we should still allow 'DROP TABLE' to pass analysis. Otherwise, you may be unable to drop tables that are in a bad state. Testing: - Updates existing Kudu tests to reflect the new behavior, and fixes a couple of problems with those tests that were causing them to pass spuriously (as well as fixing the same problem with another test in the file while I'm here). Change-Id: I6b41fc3c0e95508ab67f1d420b033b02ec75a5da Reviewed-on: http://gerrit.cloudera.org:8080/5144 Reviewed-by: Alex Behm <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/7bcb51b1 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/7bcb51b1 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/7bcb51b1 Branch: refs/heads/master Commit: 7bcb51b152ac0b2e7660ec96b1ee94e5ab906abd Parents: 8d4f8d8 Author: Thomas Tauber-Marshall <[email protected]> Authored: Fri Nov 18 14:08:55 2016 -0800 Committer: Internal Jenkins <[email protected]> Committed: Fri Dec 2 21:58:03 2016 +0000 ---------------------------------------------------------------------- .../java/org/apache/impala/analysis/Analyzer.java | 18 +++++++++--------- .../impala/analysis/DropTableOrViewStmt.java | 13 ++++++++++++- .../apache/impala/analysis/PartitionSpecBase.java | 8 +++++++- .../apache/impala/service/CatalogOpExecutor.java | 4 ++++ .../apache/impala/analysis/AnalyzeDDLTest.java | 3 +++ .../org/apache/impala/analysis/AuditingTest.java | 6 ++++++ tests/query_test/test_kudu.py | 16 ++++++++-------- 7 files changed, 49 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java index d4fe88d..96ab097 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java +++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java @@ -2346,7 +2346,7 @@ public class Analyzer { * If addAccessEvent is true, adds an access event if the catalog access succeeded. */ public Table getTable(TableName tableName, Privilege privilege, boolean addAccessEvent) - throws AnalysisException { + throws AnalysisException, TableLoadingException { Preconditions.checkNotNull(tableName); Preconditions.checkNotNull(privilege); Table table = null; @@ -2359,13 +2359,7 @@ public class Analyzer { registerPrivReq(new PrivilegeRequestBuilder() .allOf(privilege).onTable(tableName.getDb(), tableName.getTbl()).toRequest()); } - // This may trigger a metadata load, in which case we want to return the errors as - // AnalysisExceptions. - try { - table = getTable(tableName.getDb(), tableName.getTbl()); - } catch (TableLoadingException e) { - throw new AnalysisException(e.getMessage(), e); - } + table = getTable(tableName.getDb(), tableName.getTbl()); Preconditions.checkNotNull(table); if (addAccessEvent) { // Add an audit event for this access @@ -2387,7 +2381,13 @@ public class Analyzer { */ public Table getTable(TableName tableName, Privilege privilege) throws AnalysisException { - return getTable(tableName, privilege, true); + // This may trigger a metadata load, in which case we want to return the errors as + // AnalysisExceptions. + try { + return getTable(tableName, privilege, true); + } catch (TableLoadingException e) { + throw new AnalysisException(e.getMessage(), e); + } } /** http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java index 4ed03ba..d7d429c 100644 --- a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java @@ -19,8 +19,11 @@ package org.apache.impala.analysis; import org.apache.impala.authorization.Privilege; import org.apache.impala.catalog.Table; +import org.apache.impala.catalog.TableLoadingException; import org.apache.impala.catalog.View; import org.apache.impala.common.AnalysisException; +import org.apache.impala.thrift.TAccessEvent; +import org.apache.impala.thrift.TCatalogObjectType; import org.apache.impala.thrift.TDropTableOrViewParams; import org.apache.impala.thrift.TTableName; import com.google.common.base.Preconditions; @@ -85,7 +88,7 @@ public class DropTableOrViewStmt extends StatementBase { public void analyze(Analyzer analyzer) throws AnalysisException { dbName_ = analyzer.getTargetDbName(tableName_); try { - Table table = analyzer.getTable(tableName_, Privilege.DROP); + Table table = analyzer.getTable(tableName_, Privilege.DROP, true); Preconditions.checkNotNull(table); if (table instanceof View && dropTable_) { throw new AnalysisException(String.format( @@ -95,6 +98,14 @@ public class DropTableOrViewStmt extends StatementBase { throw new AnalysisException(String.format( "DROP VIEW not allowed on a table: %s.%s", dbName_, getTbl())); } + } catch (TableLoadingException e) { + // We should still try to DROP tables that failed to load, so that tables that are + // in a bad state, eg. deleted externally from Kudu, can be dropped. + // We still need an access event - we don't know if this is a TABLE or a VIEW, so + // we set it as TABLE as VIEW loading is unlikely to fail and even if it does + // TABLE -> VIEW is a small difference. + analyzer.addAccessEvent(new TAccessEvent( + tableName_.toString(), TCatalogObjectType.TABLE, Privilege.DROP.toString())); } catch (AnalysisException e) { if (ifExists_ && analyzer.getMissingTbls().isEmpty()) return; throw e; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java index 7d9a800..e76936e 100644 --- a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java +++ b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java @@ -19,6 +19,7 @@ package org.apache.impala.analysis; import org.apache.impala.authorization.Privilege; import org.apache.impala.catalog.Table; +import org.apache.impala.catalog.TableLoadingException; import org.apache.impala.catalog.HdfsTable; import org.apache.impala.common.AnalysisException; import com.google.common.base.Preconditions; @@ -76,7 +77,12 @@ public abstract class PartitionSpecBase implements ParseNode { // Skip adding an audit event when analyzing partitions. The parent table should // be audited outside of the PartitionSpec. - Table table = analyzer.getTable(tableName_, privilegeRequirement_, false); + Table table; + try { + table = analyzer.getTable(tableName_, privilegeRequirement_, false); + } catch (TableLoadingException e) { + throw new AnalysisException(e.getMessage(), e); + } // Make sure the target table is partitioned. if (table.getMetaStoreTable().getPartitionKeysSize() == 0) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java index 52000a9..b06f415 100644 --- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java +++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java @@ -1337,6 +1337,10 @@ public class CatalogOpExecutor { try (MetaStoreClient msClient = catalog_.getMetaStoreClient()) { msClient.getHiveClient().dropTable( tableName.getDb(), tableName.getTbl(), true, params.if_exists, params.purge); + } catch (NoSuchObjectException e) { + throw new ImpalaRuntimeException(String.format("Table %s no longer exists in " + + "the Hive MetaStore. Run 'invalidate metadata %s' to update the Impala " + + "catalog.", tableName, tableName)); } catch (TException e) { throw new ImpalaRuntimeException( String.format(HMS_RPC_ERROR_FORMAT_STR, "dropTable"), e); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java index 2a64b4c..4aeb96d 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java @@ -1193,6 +1193,9 @@ public class AnalyzeDDLTest extends FrontendTestBase { // Cannot drop a table with DROP VIEW. AnalysisError("drop view functional.alltypes", "DROP VIEW not allowed on a table: functional.alltypes"); + + // No analysis error for tables that can't be loaded. + AnalyzesOk("drop table functional.unsupported_partition_types"); } @Test http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java index df23c1e..163424f 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java @@ -223,6 +223,12 @@ public class AuditingTest extends AnalyzerTest { Set<TAccessEvent> accessEvents = AnalyzeAccessEvents("drop table tpch.lineitem"); Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent( "tpch.lineitem", TCatalogObjectType.TABLE, "DROP"))); + + // Dropping a table that fails loading should still result in an access event. + accessEvents = AnalyzeAccessEvents( + "drop table functional.unsupported_partition_types"); + Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent( + "functional.unsupported_partition_types", TCatalogObjectType.TABLE, "DROP"))); } @Test http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7bcb51b1/tests/query_test/test_kudu.py ---------------------------------------------------------------------- diff --git a/tests/query_test/test_kudu.py b/tests/query_test/test_kudu.py index b376ea9..9c9112e 100644 --- a/tests/query_test/test_kudu.py +++ b/tests/query_test/test_kudu.py @@ -434,7 +434,7 @@ class TestDropDb(KuduTestSuite): unique_cursor.execute("USE DEFAULT") unique_cursor.execute("DROP DATABASE %s CASCADE" % db_name) unique_cursor.execute("SHOW DATABASES") - assert db_name not in unique_cursor.fetchall() + assert (db_name, '') not in unique_cursor.fetchall() assert kudu_client.table_exists(kudu_table.name) assert not kudu_client.table_exists(managed_table_name) @@ -477,8 +477,7 @@ class TestImpalaKuduIntegration(KuduTestSuite): def test_delete_external_kudu_table(self, cursor, kudu_client): """Check that Impala can recover from the case where the underlying Kudu table of - an external table is dropped using the Kudu client. The external table can be - dropped using DROP TABLE IF EXISTS statement. + an external table is dropped using the Kudu client. """ with self.temp_kudu_table(kudu_client, [INT32]) as kudu_table: # Create an external Kudu table @@ -498,9 +497,10 @@ class TestImpalaKuduIntegration(KuduTestSuite): cursor.execute("REFRESH %s" % (impala_table_name)) except Exception as e: assert err_msg in str(e) - cursor.execute("DROP TABLE IF EXISTS %s" % (impala_table_name)) + cursor.execute("DROP TABLE %s" % (impala_table_name)) cursor.execute("SHOW TABLES") - assert impala_table_name not in cursor.fetchall() + assert (impala_table_name,) not in cursor.fetchall() + def test_delete_managed_kudu_table(self, cursor, kudu_client, unique_database): """Check that dropping a managed Kudu table works even if the underlying Kudu table @@ -512,9 +512,9 @@ class TestImpalaKuduIntegration(KuduTestSuite): assert kudu_client.table_exists(kudu_tbl_name) kudu_client.delete_table(kudu_tbl_name) assert not kudu_client.table_exists(kudu_tbl_name) - cursor.execute("DROP TABLE IF EXISTS %s" % (impala_tbl_name)) - cursor.execute("SHOW TABLES") - assert impala_tbl_name not in cursor.fetchall() + cursor.execute("DROP TABLE %s.%s" % (unique_database, impala_tbl_name)) + cursor.execute("SHOW TABLES IN %s" % unique_database) + assert (impala_tbl_name,) not in cursor.fetchall() class TestKuduMemLimits(KuduTestSuite):
