Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13746 )
Change subject: IMPALA-8717: impala-shell support for HS2 HTTP endpoint ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java: http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@133 PS6, Line 133: } catch (AssertionError e) { : LOG.error("Failed to run cmd: {} stdout: {}", cmd, stdout); Instead of catching the assertion failure to log and rethrow it, I think you can just pass the error message in as the first parameter to assertEquals() http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@140 PS6, Line 140: BASIC nit: I probably got this wrong in some places in my patches around this, but this shouldn't all be capitalized http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@143 PS6, Line 143: public void testShellBasicAuth() throws Exception { We don't really want to inherit all of the JDBC related setup stuff here, eg. currently its getting run twice cause JdbcTestBase is parameterized for binary vs. http, so probably better to put this in its own class. http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@149 PS6, Line 149: show tables Could you make the 'select logged_in_user()' and actually verify that its correct? http://gerrit.cloudera.org:8080/#/c/13746/6/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/13746/6/shell/impala_shell.py@804 PS6, Line 804: print_to_stderr("protocol: " + options.protocol.lower()) : assert options.protocol.lower() == 'beeswax' Probably only want to log if its actually wrong, and we should probably return a FatalShellException() in that case instead of assert-ing. -- To view, visit http://gerrit.cloudera.org:8080/13746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8323950857dfe1c1dfd5377fde79f87bc2ce9534 Gerrit-Change-Number: 13746 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 22 Jul 2019 21:18:29 +0000 Gerrit-HasComments: Yes
