Alex Behm has posted comments on this change.

Change subject: IMPALA-1702: Enforce table level consistency accross service

Patch Set 3:

File fe/src/main/java/org/apache/impala/analysis/

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.
  return table;
File fe/src/main/java/org/apache/impala/catalog/

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.
File fe/src/main/java/org/apache/impala/catalog/

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.
File fe/src/main/java/org/apache/impala/common/

Line 42:     return Long.valueOf(id_).hashCode();

Line 47:     return Long.toString(id_);
File fe/src/main/java/org/apache/impala/service/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Huaisi Xu <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Huaisi Xu <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

Reply via email to