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 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/13746/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13746/5//COMMIT_MSG@29 PS5, Line 29: - Manually tested plain LDAP and LDAP + TLS combinations due to lack If you're interested, it should be possible to write automated tests for this using the LDAP support used by LdapJdbcTest.java Since we don't have any existing impala-shell/LDAP tests, it isn't really a testing regression and probably not a requirement for getting this patch in. http://gerrit.cloudera.org:8080/#/c/13746/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13746/5/be/src/service/impala-server.cc@2081 PS5, Line 2081: << disconnected_sessions.size() << " associated session(s) closed."; I feel like the "with" ... " sessions closed" makes it sound like the sessions themselves were also closed, which isn't the case. Maybe something like: Connection ... to server ... closed. The connection had 'n' associated sessions. or similar http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_client.py@367 PS5, Line 367: if connect_timeout_ms > 0: What would happen if you set the socket timeout and then just didn't reset it to 'None' like you do in _get_transport() so that it applies to every connect attempt? http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_shell.py@531 PS5, Line 531: False Maybe include the param name here and below, i.e. "use_http_base_transport=False" to make it clearer what's being set. http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_shell.py@544 PS5, Line 544: or nit: comma http://gerrit.cloudera.org:8080/#/c/13746/5/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/13746/5/tests/run-tests.py@234 PS5, Line 234: webserver_certificate_file=cert).read_debug_webpage('metrics?json')) Why is this needed? http://gerrit.cloudera.org:8080/#/c/13746/5/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/13746/5/tests/shell/test_shell_interactive.py@268 PS5, Line 268: target_port = 21001 assert protocol == "beeswax" -- 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: 5 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 18 Jul 2019 18:50:58 +0000 Gerrit-HasComments: Yes
