Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13938 )
Change subject: IMPALA-8600 Refresh transactional tables ...................................................................... Patch Set 4: (10 comments) Seems good to me, only found a couple of nits http://gerrit.cloudera.org:8080/#/c/13938/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13938/4//COMMIT_MSG@20 PS4, Line 20: Similary nit: Similarly http://gerrit.cloudera.org:8080/#/c/13938/4//COMMIT_MSG@23 PS4, Line 23: changes like adding a column neither on table level or on partition Offline you mentioned lastDdlTime. Maybe you can add a comment or TODO about it. It's also OK to wait for Hive to fix the writeId bug. http://gerrit.cloudera.org:8080/#/c/13938/4//COMMIT_MSG@28 PS4, Line 28: updates the table level writeId even for schema changes. You mentioned that it is a Hive-bug. Do we have an open Jira for that? http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@161 PS4, Line 161: Refresh nit: Refreshing a partition / REFRESH on a partition ? http://gerrit.cloudera.org:8080/#/c/13938/4/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/13938/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1900 PS4, Line 1900: nit: Please add comment http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@733 PS4, Line 733: org.apache.hadoop.hive.metastore.api.Partition We imported it above. http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1370 PS4, Line 1370: nit: on http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1473 PS4, Line 1473: nit: on http://gerrit.cloudera.org:8080/#/c/13938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1565 PS4, Line 1565: // Reload the whole table if it's a transactional table. : if (AcidUtils.isTransactionalTable(msTbl_.getParameters())) { : if (!catalog_.reloadTableIfExists(dbName_, tblName_, : "processing DROP_PARTITION event from HMS")) { : debugLog("Automatic refresh table {} failed as the table is not " : + "present either in catalog or metastore.", getFullyQualifiedTblName()); : } else { : infoLog("Table {} has been refreshed after drop_partition.", : getFullyQualifiedTblName()); : } : } else { nit: the above changes are quite similar. Can we put them into some helper method? http://gerrit.cloudera.org:8080/#/c/13938/4/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/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3611 PS4, Line 3611: isTableLoadedInCatalog nit: I think this variable became unnecessary. -- 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: 4 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 30 Jul 2019 14:37:32 +0000 Gerrit-HasComments: Yes
