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 5: (2 comments) 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 We do that sometimes, its definitely appropriate. Also, I know you already have a patch out to fix this, but could you leave a TODO saying that we need to properly handle HMS integration here? Its good practice to avoid forgetting things, and it lets the reviewers know that this was intentional. 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 will try to use this outside this package (which would break the filtering on package name in run-all-tests.sh). Could you instead just move the function you added to an equivalent package-private utility class in the 'customservice' package? -- 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: 5 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: Fri, 24 May 2019 21:51:06 +0000 Gerrit-HasComments: Yes
