Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14059 )
Change subject: IMPALA-8863: Add support to run tests over HTTP/HS2 ...................................................................... Patch Set 14: (11 comments) Addressed all comments in PS16 http://gerrit.cloudera.org:8080/#/c/14059/8/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/14059/8/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@175 PS8, Line 175: RunShellCommand.Run(command, /* shouldSucceed */ false, "", > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/14059/14/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/14059/14/tests/common/impala_connection.py@323 PS14, Line 323: if 'NoneType' not in str(e): > Can we restrict this to HTTP connections only? Would be nice not to swallow Done http://gerrit.cloudera.org:8080/#/c/14059/8/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/14059/8/tests/common/impala_test_suite.py@237 PS8, Line 237: h > flake8: E713 test for membership should be 'not in' Done http://gerrit.cloudera.org:8080/#/c/14059/14/tests/common/test_dimensions.py File tests/common/test_dimensions.py: http://gerrit.cloudera.org:8080/#/c/14059/14/tests/common/test_dimensions.py@117 PS14, Line 117: # IMPALA-8864: Older python versions do not support SSLContext object that the thrift http client > nit: long line Done http://gerrit.cloudera.org:8080/#/c/14059/8/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/14059/8/tests/shell/test_shell_interactive.py@28 PS8, Line 28: from time import sleep > flake8: F401 'contextlib.contextmanager' imported but unused Done http://gerrit.cloudera.org:8080/#/c/14059/14/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/14059/14/tests/shell/test_shell_interactive.py@293 PS14, Line 293: self.create_impala_clients() > Should this be in a finally? So that a failure here doesn't cause follow-on Done http://gerrit.cloudera.org:8080/#/c/14059/14/tests/shell/test_shell_interactive.py@335 PS14, Line 335: self.create_impala_clients() > Same here (I think I created this problem in the first place by closing the This one is already inside the finally. Would you prefer to wrap it in an additional level to make it robust against failures in the run_shell commands before? http://gerrit.cloudera.org:8080/#/c/14059/8/tests/util/run_impyla_http_query.py File tests/util/run_impyla_http_query.py: http://gerrit.cloudera.org:8080/#/c/14059/8/tests/util/run_impyla_http_query.py@30 PS8, Line 30: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/14059/8/tests/util/run_impyla_http_query.py@33 PS8, Line 33: I > flake8: F841 local variable 'url' is assigned to but never used Done http://gerrit.cloudera.org:8080/#/c/14059/8/tests/util/run_impyla_http_query.py@46 PS8, Line 46: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/14059/8/tests/util/run_impyla_http_query.py@57 PS8, Line 57: > flake8: E305 expected 2 blank lines after class or function definition, fou Done -- To view, visit http://gerrit.cloudera.org:8080/14059 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7156558071781378fcb9c8941c0f4dd82eb0d018 Gerrit-Change-Number: 14059 Gerrit-PatchSet: 14 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 03 Oct 2019 17:28:47 +0000 Gerrit-HasComments: Yes
