Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13938 )
Change subject: IMPALA-8600 Refresh transactional tables ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/13938/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13938/6//COMMIT_MSG@7 PS6, Line 7: IMPALA-8600 Refresh transactional tables nit: add : for sake of consistency with other commits http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3531 PS6, Line 3531: getMetaStoreTable About the name: "get" does tell me whether the table is get from metastore or we just convert "Table tbl" to org.apache.hadoop.hive.metastore.api.Table. I would prefer something like "getTableFromMetaStore" or "fetchMetaStoreTable". http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3532 PS6, Line 3532: tbl If I see correctly tbl is only used in a precondition check. http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3626 PS6, Line 3626: if (hmsTbl == null) { : throw new CatalogException("Unable to get table " + I am 100% sure, but it makes sense to me to throw the exception at line 3671 similarly to other cases of "missing tables". http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3628 PS6, Line 3628: tblName.toString() + " from HMS. A concurrent operation might " + : "have removed it."); nit: indentation +4 http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3632 PS6, Line 3632: if (!hdfsTable.isPartitioned() && Is this path tested? Even if there are "unneeded refreshes" among the ACID tests, I would add one explicitly to test REFRESH on an unchanged table, e.g. in https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/acid.test http://gerrit.cloudera.org:8080/#/c/13938/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3635 PS6, Line 3635: // No need to refresh the table if the local writeId equals to the It could be mentioned here that Hive seems to increase writeId for metadata operations too -- To view, visit http://gerrit.cloudera.org:8080/13938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1851da22452074dbe253bcdd97145e06c7552cd3 Gerrit-Change-Number: 13938 Gerrit-PatchSet: 6 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 31 Jul 2019 13:45:05 +0000 Gerrit-HasComments: Yes
