Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14398 )
Change subject: IMPALA-8974: Fixed a bug when create kudu managed table without HMS config ...................................................................... Patch Set 18: (6 comments) I forgot to get back to this review. Thanks for cleaning up the test. I have a few style nits about the test, then I'm OK to +1. Would be good if quanlong could also confirm that you addressed his comments http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java File fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java: http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java@63 PS18, Line 63: client.execQuery("DROP TABLE default.test_kudu_without_hms"); Can you put this in a finally {} block so that the table is cleaned up even if the test fails. http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java@84 PS18, Line 84: generate_cmd nit: generateCmd http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java@103 PS18, Line 103: envp_array nit: envpArray http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java@106 PS18, Line 106: restart_cmd nit: restartCmd http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java@107 PS18, Line 107: cmds_.append(restart_cmd); It would be better to declare cmds and envp as local variables, and pass them into the methods explicitly. It's confusing having the values implicitly passed to generateHiveSiteWithoutHMS() and getCmdsArray(). http://gerrit.cloudera.org:8080/#/c/14398/18/fe/src/test/java/org/apache/impala/catalog/CreateKuduTableWithoutHMSTest.java@128 PS18, Line 128: cmds_array nit: cmdsArray -- To view, visit http://gerrit.cloudera.org:8080/14398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacc53801a660c033869cb4747910c98a80e08297 Gerrit-Change-Number: 14398 Gerrit-PatchSet: 18 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Tue, 17 Dec 2019 21:50:16 +0000 Gerrit-HasComments: Yes
