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 4: (10 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? 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 integration is enabled; if so, validate that it is integrated with the same Hive Metastore that Impala is configured to use." 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? 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 the strings? E.g. if order changes or something? 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 exists in Kudu and Impala. Perhaps "Kudu is integrated with a different Hive Metastore than that used by Impala, Kudu is ..." 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 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 might be nice to annotate the name. 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 this is now a different file. Could we have kept it in the existing file? 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? 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 new storage handler type, right? -- 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 <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: Wed, 22 May 2019 01:25:01 +0000 Gerrit-HasComments: Yes
