You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2019/06/18 16:39:46 UTC
[impala] 01/05: IMPALA-8459. LocalCatalog: Allow dropping tables
that fail to load
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 7af2eae6b82dcc1ce5a592901a8bd706a7dff685
Author: Todd Lipcon <to...@apache.org>
AuthorDate: Fri Jun 7 01:20:47 2019 -0700
IMPALA-8459. LocalCatalog: Allow dropping tables that fail to load
This adds support in LocalCatalog for IncompleteTables. A new
FeIncompleteTable interface is introduced, and when a table fails to
load, it produces a FeIncompleteTable instead of an immediate error.
This allows DROP TABLE statements to get past analysis and execute even
when tables are in bad states (eg a Kudu table with no backing table in
Kudu itself).
This re-enables some tests that were previously disabled for
LocalCatalog.
Change-Id: Ia85d6b10669866dcc9f2bf7385a4775e483dd6b0
Reviewed-on: http://gerrit.cloudera.org:8080/13557
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../java/org/apache/impala/analysis/Analyzer.java | 6 +--
.../apache/impala/analysis/StmtMetadataLoader.java | 8 ++--
.../apache/impala/catalog/FeIncompleteTable.java | 27 ++++++++++++
.../org/apache/impala/catalog/IncompleteTable.java | 7 ++-
.../impala/catalog/local/FailedLoadLocalTable.java | 50 ++++++++++++++++++++++
.../org/apache/impala/catalog/local/LocalDb.java | 17 ++++++--
.../impala/catalog/local/LocalKuduTable.java | 12 +++---
.../apache/impala/catalog/local/LocalTable.java | 19 ++++++--
.../java/org/apache/impala/service/MetadataOp.java | 3 +-
.../impala/catalog/local/LocalCatalogTest.java | 9 ++--
tests/common/skip.py | 7 ---
tests/query_test/test_kudu.py | 4 +-
12 files changed, 133 insertions(+), 36 deletions(-)
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 45b8046..6aa29d9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -47,10 +47,10 @@ import org.apache.impala.catalog.FeDataSourceTable;
import org.apache.impala.catalog.FeDb;
import org.apache.impala.catalog.FeFsTable;
import org.apache.impala.catalog.FeHBaseTable;
+import org.apache.impala.catalog.FeIncompleteTable;
import org.apache.impala.catalog.FeKuduTable;
import org.apache.impala.catalog.FeTable;
import org.apache.impala.catalog.FeView;
-import org.apache.impala.catalog.IncompleteTable;
import org.apache.impala.catalog.TableLoadingException;
import org.apache.impala.catalog.Type;
import org.apache.impala.common.AnalysisException;
@@ -2536,10 +2536,10 @@ public class Analyzer {
}
}
Preconditions.checkState(table.isLoaded());
- if (table instanceof IncompleteTable) {
+ if (table instanceof FeIncompleteTable) {
// If there were problems loading this table's metadata, throw an exception
// when it is accessed.
- ImpalaException cause = ((IncompleteTable) table).getCause();
+ ImpalaException cause = ((FeIncompleteTable) table).getCause();
if (cause instanceof TableLoadingException) throw (TableLoadingException) cause;
throw new TableLoadingException("Missing metadata for table: " + tableName, cause);
}
diff --git a/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java b/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
index 1898da5..fd11af1 100644
--- a/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
+++ b/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
@@ -26,11 +26,8 @@ import java.util.Set;
import org.apache.impala.catalog.FeCatalog;
import org.apache.impala.catalog.FeDb;
-import org.apache.impala.catalog.FeFsTable;
import org.apache.impala.catalog.FeTable;
import org.apache.impala.catalog.FeView;
-import org.apache.impala.catalog.HdfsPartition;
-import org.apache.impala.catalog.PrunablePartition;
import org.apache.impala.common.InternalException;
import org.apache.impala.compat.MetastoreShim;
import org.apache.impala.service.Frontend;
@@ -169,6 +166,9 @@ public class StmtMetadataLoader {
boolean issueLoadRequest = true;
// Loop until all the missing tables are loaded in the Impalad's catalog cache.
// In every iteration of this loop we wait for one catalog update to arrive.
+ //
+ // This isn't relevant for LocalCatalog, since we loaded all of the table references
+ // on-demand in the first recursive call to 'getMissingTables' above.
while (!missingTbls.isEmpty()) {
if (issueLoadRequest) {
catalog.prioritizeLoad(missingTbls);
@@ -177,8 +177,6 @@ public class StmtMetadataLoader {
}
// Catalog may have been restarted, always use the latest reference.
- // TODO(todd) is this necessary in the case of the LocalCatalog impl?
- // or maybe the whole loadTables() function is unnecessarily complex in that case?
FeCatalog currCatalog = fe_.getCatalog();
boolean hasCatalogRestarted = currCatalog != catalog;
if (hasCatalogRestarted && LOG.isWarnEnabled()) {
diff --git a/fe/src/main/java/org/apache/impala/catalog/FeIncompleteTable.java b/fe/src/main/java/org/apache/impala/catalog/FeIncompleteTable.java
new file mode 100644
index 0000000..b688a10
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/catalog/FeIncompleteTable.java
@@ -0,0 +1,27 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.impala.catalog;
+
+import org.apache.impala.common.ImpalaException;
+
+/**
+ * Frontend interface for representing a table that failed to load.
+ */
+public interface FeIncompleteTable extends FeTable {
+ /** Return the cause of the failure to load */
+ public ImpalaException getCause();
+}
diff --git a/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java b/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java
index 7de3d89..ac2930e 100644
--- a/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java
@@ -40,8 +40,12 @@ import com.google.common.collect.Lists;
* Represents a table with incomplete metadata. The metadata may be incomplete because
* it has not yet been loaded or because of errors encountered during the loading
* process.
+ *
+ * NOTE: this is used on the catalogd (CatalogServiceCatalog) and on the "v1"
+ * ImpaladCatalog. LocalCatalog does not use this, and instead uses
+ * FailedLoadLocalTable to represent a failed table.
*/
-public class IncompleteTable extends Table {
+public class IncompleteTable extends Table implements FeIncompleteTable {
// The cause for the incomplete metadata. If there is no cause given (cause_ = null),
// then this is assumed to be an uninitialized table (table that does not have
// its metadata loaded).
@@ -57,6 +61,7 @@ public class IncompleteTable extends Table {
* Returns the cause (ImpalaException) which led to this table's metadata being
* incomplete.
*/
+ @Override
public ImpalaException getCause() { return cause_; }
/**
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java b/fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java
new file mode 100644
index 0000000..f245c42
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/catalog/local/FailedLoadLocalTable.java
@@ -0,0 +1,50 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.impala.catalog.local;
+
+import java.util.Set;
+
+import org.apache.impala.catalog.FeIncompleteTable;
+import org.apache.impala.catalog.TableLoadingException;
+import org.apache.impala.common.ImpalaException;
+import org.apache.impala.thrift.TTableDescriptor;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * FeTable implementation which represents a table that failed to load in the
+ * LocalCatalog catalog implementation. This is used so that statements like
+ * DROP TABLE can get a handle to a table object even when that table is in some
+ * unloadable state.
+ */
+public class FailedLoadLocalTable extends LocalTable implements FeIncompleteTable {
+ private final TableLoadingException cause_;
+
+ public FailedLoadLocalTable(LocalDb db, String tblName, TableLoadingException cause) {
+ super(db, tblName);
+ this.cause_ = Preconditions.checkNotNull(cause);
+ }
+
+ @Override
+ public ImpalaException getCause() { return cause_; }
+
+ @Override
+ public TTableDescriptor toThriftDescriptor(int tableId,
+ Set<Long> referencedPartitions) {
+ throw new RuntimeException("Not serializable as descriptor");
+ }
+}
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
index 20d84f5..ef1107b 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
@@ -34,6 +34,7 @@ import org.apache.impala.catalog.FeKuduTable;
import org.apache.impala.catalog.FeTable;
import org.apache.impala.catalog.Function;
import org.apache.impala.catalog.Function.CompareMode;
+import org.apache.impala.catalog.TableLoadingException;
import org.apache.impala.thrift.TDatabase;
import org.apache.impala.thrift.TFunctionCategory;
import org.apache.impala.util.FunctionUtils;
@@ -62,7 +63,7 @@ class LocalDb implements FeDb {
* Map from lower-cased table name to table object. Values will be
* null for tables which have not yet been loaded.
*/
- private Map<String, LocalTable> tables_;
+ private Map<String, FeTable> tables_;
/**
* Map of function name to list of signatures for that function name.
@@ -110,10 +111,18 @@ class LocalDb implements FeDb {
// Table doesn't exist.
return null;
}
- LocalTable tbl = tables_.get(tblName);
+ FeTable tbl = tables_.get(tblName);
if (tbl == null) {
// The table exists but hasn't been loaded yet.
- tbl = LocalTable.load(this, tblName);
+ try{
+ tbl = LocalTable.load(this, tblName);
+ } catch (TableLoadingException tle) {
+ // If the table fails to load (eg a Kudu table that doesn't have
+ // a backing table, or some other catalogd-side issue), turn it into
+ // an IncompleteTable. This allows statements like DROP TABLE to still
+ // analyze.
+ tbl = new FailedLoadLocalTable(this, tblName, tle);
+ }
tables_.put(tblName, tbl);
}
return tbl;
@@ -146,7 +155,7 @@ class LocalDb implements FeDb {
*/
private void loadTableNames() {
if (tables_ != null) return;
- Map<String, LocalTable> newMap = new HashMap<>();
+ Map<String, FeTable> newMap = new HashMap<>();
try {
List<String> names = catalog_.getMetaProvider().loadTableNames(name_);
for (String tableName : names) {
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
index 3ba2db3..418d009 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
@@ -31,6 +31,7 @@ import org.apache.impala.catalog.FeCatalogUtils;
import org.apache.impala.catalog.FeKuduTable;
import org.apache.impala.catalog.KuduColumn;
import org.apache.impala.catalog.KuduTable;
+import org.apache.impala.catalog.TableLoadingException;
import org.apache.impala.catalog.local.MetaProvider.TableMetaRef;
import org.apache.impala.common.ImpalaRuntimeException;
import org.apache.impala.thrift.TKuduTable;
@@ -56,7 +57,8 @@ public class LocalKuduTable extends LocalTable implements FeKuduTable {
* Create a new instance based on the table metadata 'msTable' stored
* in the metastore.
*/
- static LocalTable loadFromKudu(LocalDb db, Table msTable, TableMetaRef ref) {
+ static LocalTable loadFromKudu(LocalDb db, Table msTable, TableMetaRef ref)
+ throws TableLoadingException {
Preconditions.checkNotNull(db);
Preconditions.checkNotNull(msTable);
String fullTableName = msTable.getDbName() + "." + msTable.getTableName();
@@ -212,15 +214,15 @@ public class LocalKuduTable extends LocalTable implements FeKuduTable {
return Lists.newArrayList(masters_.split(","));
}
- public org.apache.kudu.client.KuduTable openTable() {
+ public org.apache.kudu.client.KuduTable openTable() throws TableLoadingException {
KuduClient kuduClient = KuduUtil.getKuduClient(masters_);
org.apache.kudu.client.KuduTable kuduTable;
try {
kuduTable = kuduClient.openTable(kuduTableName_);
} catch (KuduException e) {
- throw new LocalCatalogException(
- String.format("Error opening Kudu table '%s', Kudu error: %s",
- kuduTableName_, e.getMessage()));
+ throw new TableLoadingException(
+ String.format("Error opening Kudu table '%s', Kudu error: %s",
+ kuduTableName_, e.getMessage()));
}
return kuduTable;
}
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
index edb0e06..907ab03 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
@@ -82,7 +82,7 @@ abstract class LocalTable implements FeTable {
@Nullable
protected final TableMetaRef ref_;
- public static LocalTable load(LocalDb db, String tblName) {
+ public static LocalTable load(LocalDb db, String tblName) throws TableLoadingException {
// In order to know which kind of table subclass to instantiate, we need
// to eagerly grab and parse the top-level Table object from the HMS.
LocalTable t = null;
@@ -119,14 +119,15 @@ abstract class LocalTable implements FeTable {
/**
* Load the Table instance from the metastore.
*/
- private static Pair<Table, TableMetaRef> loadTableMetadata(LocalDb db, String tblName) {
+ private static Pair<Table, TableMetaRef> loadTableMetadata(LocalDb db, String tblName)
+ throws TableLoadingException {
Preconditions.checkArgument(tblName.toLowerCase().equals(tblName));
try {
return db.getCatalog().getMetaProvider().loadTable(db.getName(), tblName);
} catch (TException e) {
- throw new LocalCatalogException(String.format(
- "Could not load table %s.%s from metastore",
+ throw new TableLoadingException(String.format(
+ "Could not load table %s.%s from catalog",
db.getName(), tblName), e);
}
}
@@ -148,6 +149,16 @@ abstract class LocalTable implements FeTable {
this(db, msTbl, ref, ColumnMap.fromMsTable(msTbl));
}
+ protected LocalTable(LocalDb db, String tblName) {
+ this.db_ = Preconditions.checkNotNull(db);
+ this.name_ = tblName;
+ this.ref_ = null;
+ this.msTable_ = null;
+ this.cols_ = null;
+ this.tableStats_ = null;
+ }
+
+
@Override
public boolean isLoaded() {
return true;
diff --git a/fe/src/main/java/org/apache/impala/service/MetadataOp.java b/fe/src/main/java/org/apache/impala/service/MetadataOp.java
index 0686ad99..349ee9f 100644
--- a/fe/src/main/java/org/apache/impala/service/MetadataOp.java
+++ b/fe/src/main/java/org/apache/impala/service/MetadataOp.java
@@ -31,6 +31,7 @@ import org.apache.impala.catalog.Catalog;
import org.apache.impala.catalog.Column;
import org.apache.impala.catalog.FeCatalog;
import org.apache.impala.catalog.FeDb;
+import org.apache.impala.catalog.FeIncompleteTable;
import org.apache.impala.catalog.FeTable;
import org.apache.impala.catalog.Function;
import org.apache.impala.catalog.MapType;
@@ -313,7 +314,7 @@ public class MetadataOp {
// If the table is not yet loaded, the columns will be unknown. Add it
// to the set of missing tables.
String tableType = TABLE_TYPE_TABLE;
- if (!table.isLoaded()) {
+ if (!table.isLoaded() || table instanceof FeIncompleteTable) {
result.missingTbls.add(new TableName(db.getName(), tabName));
} else {
if (table.getMetaStoreTable() != null) {
diff --git a/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java b/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
index 990577b..3fccc91 100644
--- a/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
@@ -22,6 +22,7 @@ import static org.junit.Assert.*;
import java.util.List;
import java.util.Set;
+import org.apache.hive.service.rpc.thrift.TGetSchemasReq;
import org.apache.hive.service.rpc.thrift.TGetTablesReq;
import org.apache.impala.analysis.Expr;
import org.apache.impala.analysis.ToSqlUtils;
@@ -358,8 +359,8 @@ public class LocalCatalogTest {
}
/**
- * Test GET_TABLES request on an Impala incompatible table. It should be silently
- * ignored.
+ * Test GET_TABLES request on an Impala incompatible table. The table should be
+ * listed even though it failed to load.
*/
@Test
public void testGetTables() throws Exception {
@@ -369,6 +370,8 @@ public class LocalCatalogTest {
req.get_tables_req.setSchemaName("functional");
req.get_tables_req.setTableName("bad_serde");
TResultSet resp = fe_.execHiveServer2MetadataOp(req);
- assertEquals(0, resp.rows.size());
+ assertEquals(1, resp.rows.size());
+ assertEquals(resp.rows.get(0).colVals.get(1).string_val, "functional");
+ assertEquals(resp.rows.get(0).colVals.get(2).string_val, "bad_serde");
}
}
diff --git a/tests/common/skip.py b/tests/common/skip.py
index 81c004d..3541bbd 100644
--- a/tests/common/skip.py
+++ b/tests/common/skip.py
@@ -261,13 +261,6 @@ class SkipIfCatalogV2:
reason="IMPALA-8489: TestRecoverPartitions.test_post_invalidate "
"IllegalStateException.")
- # TODO: IMPALA-8459: fix this bug.
- @classmethod
- def impala_8459(self):
- return pytest.mark.skipif(
- IMPALA_TEST_CLUSTER_PROPERTIES.is_catalog_v2_cluster(),
- reason="IMPALA-8459: some kudu DDL is broken for local catalog")
-
# TODO: IMPALA-7539: fix this bug.
@classmethod
def impala_7539(self):
diff --git a/tests/query_test/test_kudu.py b/tests/query_test/test_kudu.py
index df347c6..027a291 100644
--- a/tests/query_test/test_kudu.py
+++ b/tests/query_test/test_kudu.py
@@ -40,7 +40,7 @@ from pytz import utc
from tests.common.environ import IMPALA_TEST_CLUSTER_PROPERTIES
from tests.common.kudu_test_suite import KuduTestSuite
from tests.common.impala_cluster import ImpalaCluster
-from tests.common.skip import SkipIfNotHdfsMinicluster, SkipIfKudu, SkipIfCatalogV2
+from tests.common.skip import SkipIfNotHdfsMinicluster, SkipIfKudu
from tests.common.test_dimensions import add_exec_option_dimension
from tests.verifiers.metric_verifier import MetricVerifier
@@ -1097,7 +1097,6 @@ class TestImpalaKuduIntegration(KuduTestSuite):
("c", "string", "", "false", "true", "", "AUTO_ENCODING",
"DEFAULT_COMPRESSION", "0")]
- @SkipIfCatalogV2.impala_8459()
@SkipIfKudu.hms_integration_enabled
def test_delete_external_kudu_table(self, cursor, kudu_client):
"""Check that Impala can recover from the case where the underlying Kudu table of
@@ -1125,7 +1124,6 @@ class TestImpalaKuduIntegration(KuduTestSuite):
cursor.execute("SHOW TABLES")
assert (impala_table_name,) not in cursor.fetchall()
- @SkipIfCatalogV2.impala_8459()
@SkipIfKudu.hms_integration_enabled
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