Andrew Wong 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 6: (4 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@192 PS4, Line 192: public static boolean isHMSIntegrationEnabledAndValidate(String kuduMasters, > No, it is used in the follow up patch. I see. I'm fine with keeping this in if it's a hassle for you to rebase, but it would have been nice to separate it out early since 1) as you note, this patch is already quite large, and 2) this function not being used also implies there isn't testing for it, and 3) without seeing how it's used, in reviewing, it's hard to determine whether it looks right. 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) > Yeah, I think it will be nice to check that, though I would prefer to add a SGTM. Will that be in the follow-up patch where this function is used or a Jira? http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java File fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java: http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java@38 PS6, Line 38: AnalyzeKuduDDLTest.class, AuditingKuduTest.class, : ParserTest.class, ToSqlTest.class Do these also get run without the HMS integration enabled? http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java@60 PS6, Line 60: final String hmsIntegrationEnv = String.format("IMPALA_KUDU_STARTUP_FLAGS=" + nit: definition could be gated by enableHMSIntegration if this were put at L66 -- 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: 6 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 28 May 2019 04:52:01 +0000 Gerrit-HasComments: Yes
