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 <[email protected]> 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 <[email protected]> Authored: Mon Feb 27 21:42:26 2017 -0800 Committer: Impala Public Jenkins <[email protected]> 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); }
