Alex Behm has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 294: // Caches all table used in a single query. Use the same reference instead of getting "Caches all tables referenced in a single query" The comment should explain why we have this local cache. The main motivation is to guarantee single table consistency within a query, i.e., that within one query the same version of a table is used. Note that the catalog cache always contains the latest version of a table. Line 2290: TTableName tblName = new TTableName(dbName, tableName); I'm not sure if this will deal with case-insensitivity properly. Why not use a TableName instead? The equals() and hashCode() definitely do the right thing. Line 2291: Table table = globalState_.referencedTables_.get(tblName); How about this: if (table != null) { // Return query-local version of table. Preconditions.checkState(table.isLoaded()); return table; } http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 141: protected final AtomicLong nextTableId_ = new AtomicLong(0); Add comment that explains the use and behavior of this id, i.e. the id is ever-increasing and only gets reset when the service is re-started. Mention at which points a table gets assigned a new id. http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: Line 65: public static final long INVALID_TABLE_ID = -1; Add comment for these subtly different special ids. Line 67: public static final long ERROR_METADATA_LOADING_ID = -3; I think we should add LOCAL_TABLE_ID and TEST_TABLE_ID as well and describe their uses. http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/common/Id.java File fe/src/main/java/org/apache/impala/common/Id.java: Line 42: return Long.valueOf(id_).hashCode(); revert Line 47: return Long.toString(id_); revert http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: Line 1130 I think we should still maintain something like this to check and document our new guarantees. In addition, it would be good to check our invariants in other places if possible. For example, when we receive an update for a table from the statestore, we should be able to guarantee that the currentTableId <= newTableId iff currentCatalogVersion < catalogVersion. Or something along those lines, I think you get the idea :) Line 1134 There is similar code in a few places in the BE. We should remove that, too. -- To view, visit http://gerrit.cloudera.org:8080/4349 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
