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 Tue, 04 Oct 2016 04:21:35 GMT
Alex Behm has posted comments on this change.

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


Patch Set 4:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 294:     // Catalog cache has the latest tables from the statestore, in order to use
the
The Impalad Catalog has ...

Sentence is pretty long, use a few "."


Line 295:     // same table version in a single query, we cache all referenced tables and
reuse them
... query , we cache all referenced tables here.


Line 2299:     if (table == null) {
remove


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java:

Line 187:       // SelectStmt (or the BE will be very confused). Assign a unique table ID
to it.
The previous longer comment was clearer. How about:

To ensure the table ID us unique within this query assign a special CTAS ID.


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java:

Line 44:   // List of tables to exclude from partition pruning, and thus all partitions are
sent to
Afaik, these tables have nothing to do with partition pruning, so the name is misleading.

We need to add register these tables so that the BE can see the table metadata in the DesciptorTable.

How about tblsWithoutTupleDesc?


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

Line 141:    * Ever increasing counter for table id {@link Table#id_}. New id is given when
TableId doesn't exist anymore.


Ever increasing counter for table ids. A table is given a new id when loading its metadata
for the first time or when doing a refresh/invalidate.


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

Line 64:   private static final Object metastoreAccessLock_ = new Object();
Please remove while you are here.


Line 72:   public static final long LOCAL_TABLE_ID = -4;
LOCAL_VIEW_ID is more explicit. Sorry to make you change it again.


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/planner/JoinTableId.java
File fe/src/main/java/org/apache/impala/planner/JoinTableId.java:

Line 25:   // Construction only allowed via an IdGenerator.
newline before comment


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

Line 1126:    * Check that we don't have any duplicate table IDs (see IMPALA-1702).
We should declare IMPALA-1702 as fixed and not mention it here. Instead, we should describe
our expectations:

1. All referenced tables have a unique id
2. All references of the same table refer to the same version (indicated by the table id)


I'd still like to see some validation when receiving updates from the Statestore. I mentioned
that in the last round and I think it's important we add that to make sure the system behaves
as expected.


-- 
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: 4
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