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 <h...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Huaisi Xu <h...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
