Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13400 )
Change subject: IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 2: (3 comments) Didn't look very deeply at the tests since this is still WIP. http://gerrit.cloudera.org:8080/#/c/13400/2/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/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1439 PS2, Line 1439: // The Kudu tables with the Hive Metastore integration enabled should : // have been dropped at this point. Shouldn't this be true regardless of HMS integration? Isn't that what dropTablesFromKudu does (I would guess)? http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1512 PS2, Line 1512: private boolean isHMSIntegrationEnabled( The CatalogOpExecutor class isn't Kudu-specific; this should probably be named to indicate that it is specific to Kudu, eg isKuduHmsIntegrationEnabled or somesuch http://gerrit.cloudera.org:8080/#/c/13400/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1613 PS2, Line 1613: if (!isManagedKuduTable || !isHMSIntegrationEnabled(msTbl)) { Please add a comment explaining the rationale of this condition, eg "When Kudu's HMS integration is turned on, Kudu will drop managed table entries automatically. In all other cases, we need to drop the HMS table entry ourselves." -- To view, visit http://gerrit.cloudera.org:8080/13400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Gerrit-Change-Number: 13400 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Thu, 23 May 2019 19:08:02 +0000 Gerrit-HasComments: Yes
