Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@171 PS4, Line 171: String.format("Error getting the configuration on whether Kudu's " + > Are these error scenarios tested? Added a test case in AnalyzeKuduDDLTest. http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@188 PS4, Line 188: Check with Kudu master to see if Kudu/HMS integration is enabled. As well : * as validate if Kudu is configured to the given Hive Metastore that Impala : * is configured to use. > nit: reword slightly "Check with the Kudu master to see if its Kudu-HMS int Done http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192 PS4, Line 192: public static boolean isHMSIntegrationEnabledAndValidate(String kuduMasters, > Is this used in this patch? No, it is used in the follow up patch. http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@200 PS4, Line 200: hmsUris.equals(kuduHMSUris) > Do you think it's important to check the set equality of addresses vs just Yeah, I think it will be nice to check that, though I would prefer to add a todo and do it in a follow up patch, to avoid putting too much stuff in this commit. http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@204 PS4, Line 204: When Kudu's integration with the Hive Metastore is enabled > nit: this reads as a general statement, rather than a statement of what exi Done http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@251 PS4, Line 251: (k > nit: extra parentheses Done http://gerrit.cloudera.org:8080/#/c/13318/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/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579 PS4, Line 2579: false > nit: not sure if this is standard practice in the Impala codebase, but migh Done http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579 PS4, Line 2579: false > We do that sometimes, its definitely appropriate. Done http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: PS4: > What has changed from AnalyzeDDLTest? It's a bit hard to determine since th This is only to extract Kudu table related test. Nothing has changed except I added a new test about 'Kudu table is not allowed to set table property 'kudu.table_id' and another negative test for getting HMS integration configuration as you requested. I extract it to a new file because it doesn't make sense to run non-kudu table related test twice with/without HMS integration. http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java: PS4: > Same here: what's changed with AuditingTest here that warrants a new file? This is only about Kudu tables and nothing has changed here. Just to run the test with/without HMS integration. http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@333 PS4, Line 333: String kuduMasters = catalog_.getDefaultKuduMasterHosts(); > Just making sure I understand this: the only necessary change here was the Yes, I also update the Kudu master host address to be a valid one to not break the test. http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java: http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@29 PS5, Line 29: public > Leaving off this 'public' was intentional to reduce the chances someone wil Sure, done. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Tue, 28 May 2019 04:10:09 +0000 Gerrit-HasComments: Yes
