Thomas Marshall 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 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@57 PS2, Line 57: private static final Logger LOG = Logger.getLogger(KuduTable.class); This isn't used anywhere http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@167 PS2, Line 167: // TODO: validate Kudu is configured to use the same HMS as Impala. How difficult would this be to implement and what's the failure mode if its not the case? http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@390 PS2, Line 390: String metastoreDbName, : String metastoreTableName, : boolean isHMSIntegrationEnabled Please try to match the formatting in the rest of the file, here and elsewhere. Eg. don't separate all parameters onto their own lines, just wrap as necessary and indent four spaces. You might want to look into clang-format-diff as linked here: https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala though note that its more of a guideline than a rule, and in places where it differs from the existing formatting you should just match what we already do http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397 PS2, Line 397: public static boolean isDefaultKuduTableName(String name, brief comment http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java File fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java@39 PS2, Line 39: public class KuduHMSIntegrationTest { This makes me nervous. We don't currently have any tests that restart any minicluster components like this. There are a lot of tests that will run after this that depend on the minicluster being set up in a particular way and we should be careful not to mess with those. I think that what we should do is essentially define a new test suite that will run at the very end. I have a patch out right now that does something similar: https://gerrit.cloudera.org/#/c/13337/ You can copy what I've done there: - Put this class in its own package, say: org.apache.impala.customservice - Copy the changes I made to bin/run-all-tests.sh with appropriate modifications - It would also be great if you could define a generic 'restartMiniclusterComponent' function in a utility class like the CustomClusterRunner class that I added that takes a component name (i.e. 'kudu') and a set of extra env variables to set and then use that in setup/cleanup http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@24 PS2, Line 24: import org.apache.impala.catalog.KuduTable; nit: not needed http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@51 PS2, Line 51: import org.apache.impala.catalog.KuduTable; nit: not needed -- 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: 2 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: Fri, 17 May 2019 21:38:32 +0000 Gerrit-HasComments: Yes
