Bharath Vissapragada 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 7: (6 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@116 PS6, Line 116: > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@133 PS6, Line 133: : > Instead of catching the assertion failure to log and rethrow it, I think yo Done http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@140 PS6, Line 140: > nit: I probably got this wrong in some places in my patches around this, bu removed it here. Basic is specific to http, but here were are testing all the protocols. http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@143 PS6, Line 143: > We don't really want to inherit all of the JDBC related setup stuff here, e ya, I should've seen that. Fixed it now. http://gerrit.cloudera.org:8080/#/c/13746/6/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java@149 PS6, Line 149: > Could you make the 'select logged_in_user()' and actually verify that its c Done 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: assert options.protocol.lower() == 'beeswax' : port = str(DEFAULT_BEESWAX_PORT) > Probably only want to log if its actually wrong, and we should probably ret oops, debugging message leaked here. thanks for catching. I don't think anyone would ever hit this assertion unless they are changing something in the code. So I think its ok to keep it as is. -- 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: 7 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: Wed, 24 Jul 2019 00:29:24 +0000 Gerrit-HasComments: Yes
