You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jr...@apache.org on 2017/03/02 19:02:14 UTC
[5/5] incubator-impala git commit: IMPALA-4998: Fix missing table
lock acquisition.
IMPALA-4998: Fix missing table lock acquisition.
The following commit broke test_views_compatibility.py
which only runs in exhaustive mode, so the issue was
not caught by pre-commit testing:
a71636847fe742a9d0eb770516aff34ff16bbca1
Testing: Before this patch test_views_compatibility.py
failed locally reliably. After this patch the test
passes locally.
Change-Id: I0e0270daf59fce95f1a1520fc5aaf91d3a7b99fe
Reviewed-on: http://gerrit.cloudera.org:8080/6177
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public 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/c4637960
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/c4637960
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/c4637960
Branch: refs/heads/master
Commit: c46379602af3c007f9f8c98d8d52b8f7dec03270
Parents: 60c1c6e
Author: Alex Behm <al...@cloudera.com>
Authored: Mon Feb 27 21:42:26 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Thu Mar 2 10:31:29 2017 +0000
----------------------------------------------------------------------
.../impala/catalog/CatalogServiceCatalog.java | 61 +++++---------------
.../apache/impala/catalog/TableLoadingMgr.java | 11 +++-
2 files changed, 23 insertions(+), 49 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c4637960/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
index 2adba9e..18ece9b 100644
--- a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
+++ b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
@@ -923,43 +923,17 @@ public class CatalogServiceCatalog extends Catalog {
}
/**
- * Reloads metadata for table 'tbl'. If 'tbl' is an IncompleteTable, it makes an
- * asynchronous request to the table loading manager to create a proper table instance
- * and load the metadata from Hive Metastore. Otherwise, it updates table metadata
- * in-place by calling the load() function on the specified table. Returns the
- * TCatalogObject representing 'tbl', if it is a fully loaded table (e.g. HdfsTable,
- * HBaseTable, etc). Otherwise, returns a newly constructed fully loaded TCatalogObject.
- * Applies proper synchronization to protect the metadata load from concurrent table
- * modifications and assigns a new catalog version.
+ * Reloads metadata for table 'tbl' which must not be an IncompleteTable. Updates the
+ * table metadata in-place by calling load() on the given table. Returns the
+ * TCatalogObject representing 'tbl'. Applies proper synchronization to protect the
+ * metadata load from concurrent table modifications and assigns a new catalog version.
* Throws a CatalogException if there is an error loading table metadata.
*/
public TCatalogObject reloadTable(Table tbl) throws CatalogException {
LOG.info(String.format("Refreshing table metadata: %s", tbl.getFullName()));
- TTableName tblName = new TTableName(tbl.getDb().getName().toLowerCase(),
- tbl.getName().toLowerCase());
- Db db = tbl.getDb();
- if (tbl instanceof IncompleteTable) {
- TableLoadingMgr.LoadRequest loadReq;
- long previousCatalogVersion;
- // Return the table if it is already loaded or submit a new load request.
- catalogLock_.readLock().lock();
- try {
- previousCatalogVersion = tbl.getCatalogVersion();
- loadReq = tableLoadingMgr_.loadAsync(tblName);
- } finally {
- catalogLock_.readLock().unlock();
- }
- Preconditions.checkNotNull(loadReq);
- try {
- // The table may have been dropped/modified while the load was in progress, so
- // only apply the update if the existing table hasn't changed.
- Table result = replaceTableIfUnchanged(loadReq.get(), previousCatalogVersion);
- return result.toTCatalogObject();
- } finally {
- loadReq.close();
- LOG.info(String.format("Refreshed table metadata: %s", tbl.getFullName()));
- }
- }
+ Preconditions.checkState(!(tbl instanceof IncompleteTable));
+ String dbName = tbl.getDb().getName();
+ String tblName = tbl.getName();
if (!tryLockTable(tbl)) {
throw new CatalogException(String.format("Error refreshing metadata for table " +
@@ -971,11 +945,10 @@ public class CatalogServiceCatalog extends Catalog {
try (MetaStoreClient msClient = getMetaStoreClient()) {
org.apache.hadoop.hive.metastore.api.Table msTbl = null;
try {
- msTbl = msClient.getHiveClient().getTable(db.getName(),
- tblName.getTable_name());
+ msTbl = msClient.getHiveClient().getTable(dbName, tblName);
} catch (Exception e) {
throw new TableLoadingException("Error loading metadata for table: " +
- db.getName() + "." + tblName.getTable_name(), e);
+ dbName + "." + tblName, e);
}
tbl.load(true, msClient.getHiveClient(), msTbl);
}
@@ -989,15 +962,6 @@ public class CatalogServiceCatalog extends Catalog {
}
/**
- * Reloads the metadata of a table with name 'tableName'.
- */
- public void reloadTable(TTableName tableName) throws CatalogException {
- Table table = getTable(tableName.getDb_name(), tableName.getTable_name());
- if (table == null) return;
- reloadTable(table);
- }
-
- /**
* Drops the partitions specified in 'partitionSet' from 'tbl'. Throws a
* CatalogException if 'tbl' is not an HdfsTable. Returns the target table.
*/
@@ -1076,7 +1040,12 @@ public class CatalogServiceCatalog extends Catalog {
Table result = removeTable(dbName, tblName);
if (result == null) return null;
tblWasRemoved.setRef(true);
- return result.toTCatalogObject();
+ result.getLock().lock();
+ try {
+ return result.toTCatalogObject();
+ } finally {
+ result.getLock().unlock();
+ }
}
db = getDb(dbName);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c4637960/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java b/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java
index 35cb902..f3abfee 100644
--- a/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java
+++ b/fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java
@@ -305,13 +305,18 @@ public class TableLoadingMgr {
}
/**
- * Executes all async refresh work for the specified table name.
+ * Reloads the metadata of the given table to pick up the new cached block location
+ * information. Only reloads the metadata if the table is already loaded. The rationale
+ * is that if the metadata has not been loaded yet, then it needs to be reloaded
+ * anyway, and if the table failed to load, then we do not want to hide errors by
+ * reloading it 'silently' in response to the completion of an HDFS caching request.
*/
private void execAsyncRefreshWork(TTableName tblName) {
if (!waitForCacheDirs(tblName)) return;
try {
- // Reload the table metadata to pickup the new cached block location information.
- catalog_.reloadTable(tblName);
+ Table tbl = catalog_.getTable(tblName.getDb_name(), tblName.getTable_name());
+ if (tbl == null || tbl instanceof IncompleteTable || !tbl.isLoaded()) return;
+ catalog_.reloadTable(tbl);
} catch (CatalogException e) {
LOG.error("Error reloading cached table: ", e);
}