Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13337 )
Change subject: IMPALA-8545: Test Ldap authentication ...................................................................... Patch Set 2: (7 comments) Thanks for fixing this. I think this fills a major test gap. I think we need to figure out some issues with test sequencing in bin/run-all-tests.sh and starting/stopping clusters. It looks like this runs as part of the FE tests, so will actually affect the cluster that the e2e tests run against. http://gerrit.cloudera.org:8080/#/c/13337/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13337/2//COMMIT_MSG@8 PS2, Line 8: Can you mention that this changes JdbcTests to start custom clusters. http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java File fe/src/test/java/org/apache/impala/service/JdbcTestBase.java: http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java@44 PS2, Line 44: public static void cleanUp() throws Exception { Brief comment? I think the important thing is that child classes should call this from their cleanUp() method. http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java@72 PS2, Line 72: protected void addTestTable(String createTableSql) throws Exception { Brief method comment? http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/LdapJdbcTest.java File fe/src/test/java/org/apache/impala/service/LdapJdbcTest.java: http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/LdapJdbcTest.java@54 PS2, Line 54: user testUser_? When reading the below code I was looking for a local variable or argument called 'user' http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/LdapJdbcTest.java@55 PS2, Line 55: password testPassword_? http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/service/LdapJdbcTest.java@73 PS2, Line 73: CustomClusterRunner.StartImpalaCluster(); I think it would be better if we changed it so either: * We have a set of JDBC end-to-end tests that don't start/stop clusters. These get run against the regular e2e cluster. Then we can run JDBC custom cluster tests with the regular custom cluster tests. * All JDBC tests start their own cluster and are treated the same as custom cluster tests. I think I prefer 1. Actually... I think as-is this introduces a major problem because this cluster will be left running after the FE tests for the e2e tests and TEST_START_CLUSTER_ARGS won't have an effect (i.e. we won't test what we think we're testing). http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/testutil/CustomClusterRunner.java File fe/src/test/java/org/apache/impala/testutil/CustomClusterRunner.java: http://gerrit.cloudera.org:8080/#/c/13337/2/fe/src/test/java/org/apache/impala/testutil/CustomClusterRunner.java@22 PS2, Line 22: public class CustomClusterRunner { Can you add a note to bin/run-all-tests.sh that JdbcTests can start custom clusters. -- To view, visit http://gerrit.cloudera.org:8080/13337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92b5e60860c60209c1bd8afe5b3ea201fb7a7513 Gerrit-Change-Number: 13337 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 15 May 2019 16:56:09 +0000 Gerrit-HasComments: Yes
