Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17697 )
Change subject: IMPALA-10801: Check the latest compaction Id before serving ACID table ...................................................................... Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2127 PS10, Line 2127: Get non-ACID table with writeIdList: This text here comes as a message for the IllegalStateException thrown and is user-visible. The message should be easy to understand for the user. I would recommend writing something like "Compaction id check cannot be done for non-transaction table + tbl.getFullName()" http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2131 PS10, Line 2131: getPartitionsForRefreshingFileMetadata It looks like we are fetching the same information twice from the metastore. Once when we want to detect if there is a compaction on any of the partitions of the table, second when we are actually updating the compactionId of the partition in the refreshFileMetadata method below. Can we avoid the repeated work? May be you can reuse the compactionIds which HMS returned in by setting them in partsToBeRefreshed so that refreshFileMetadata method below reuses them. That way you can get rid of getAndSetLastCompactionId() http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3479 PS10, Line 3479: If there is an ongoing loading task, we don't reload file metadata but wait for the : * loading task completed and return the table just loaded. this comment is stale now that removed the loadReq logic. http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@41 PS10, Line 41: Log is this import needed? http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@345 PS10, Line 345: hdfsTable_.writeLock().lock(); why is this needed? The table locks are generally taken at CatalogOpExecutor level. e.g see callers of loadTableMetadata() in CatalogOpExecutor http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/17697/10/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@115 PS10, Line 115: able.writeLock().lock(); Why is this needed? http://gerrit.cloudera.org:8080/#/c/17697/10/testdata/cluster/ranger/setup/policy_5_revised.json File testdata/cluster/ranger/setup/policy_5_revised.json: http://gerrit.cloudera.org:8080/#/c/17697/10/testdata/cluster/ranger/setup/policy_5_revised.json@8 PS10, Line 8: 5 how is this change related? If it is not can we remove it from this patch and create a separate commit for it? -- To view, visit http://gerrit.cloudera.org:8080/17697 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I86a112a77980fef7f6238978bc9668a65262101e Gerrit-Change-Number: 17697 Gerrit-PatchSet: 10 Gerrit-Owner: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Comment-Date: Wed, 28 Jul 2021 19:39:34 +0000 Gerrit-HasComments: Yes
