impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1702: Enforce table level consistency accross service
Date Sat, 01 Oct 2016 01:31:54 GMT
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 <hxu@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message