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 6: (10 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: - Added shell test coverage for LDAP auth. > If you're interested, it should be possible to write automated tests for th Done. Added some test coverage. 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: << " The connection had " << disconnected_sessions.size() > I feel like the "with" ... " sessions closed" makes it sound like the sessi Fair point. Done. 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: # TODO: Investigate connection reuse in THttpClient and revisit this. > What would happen if you set the socket timeout and then just didn't reset Yes. that is not desirable for longer duration RPCs, say waiting on results etc. http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_client.py@369 PS5, Line 369: print_to_stderr("Warning: --connect_timeout_ms is currently ignored with" + switched this to a warning. http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_client.py@392 PS5, Line 392: > flake8: E501 line too long (93 > 90 characters) Done 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: self. > Maybe include the param name here and below, i.e. "use_http_base_transport= Done http://gerrit.cloudera.org:8080/#/c/13746/5/shell/impala_shell.py@544 PS5, Line 544: _ms > nit: comma Done 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@233 PS5, Line 233: > flake8: E502 the backslash is redundant between brackets Done 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? This is not related to patch but I ran into this during my local testing. So I thought it'd nice to fix it while I'm here. So if you have a mini-cluster with TLS for web endpoints enabled, any subsequent run-tests.py command gets stuck here in a loop, trying to print metrics, because it fails to connect to the http end points without the certificate. Essentially no tests run until you manually kill the cluster or restart it without TLS for web endpoints. 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: assert protocol == "beeswax" > assert protocol == "beeswax" Done -- 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: Sat, 20 Jul 2019 03:23:38 +0000 Gerrit-HasComments: Yes
