Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 )
Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error ...................................................................... Patch Set 3: Code-Review+1 (5 comments) My main question is to confirm that we understand why the Python upgrade broke this. Before it was also broken, but we were silently ignoring that? The rest is python nits, which you can ignore if you'd like. http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG@9 PS3, Line 9: When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite To be super clear: we used to ignore this silently but now TSSLSocket.py broke this? i.e., why is it that the test started failing after the Thrift change... http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG@13 PS3, Line 13: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L37-L41 You can press "y" in the github UI to get code a permalink UI. Or specify a tag instead of "master". http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py File tests/custom_cluster/test_client_ssl.py: http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@36 PS3, Line 36: HAS_TLSV12_INCOMPATIBLE_PYTHON = True It's super confusing that you'd change a constant on line 43. Can you just put line 43 here? The same comment sort of applies to the other stuff, but you've not changed it. http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@45 PS3, Line 45: # Old ssl module versions don't even have OPENSSL_VERSION_NUMBER as a member. If you want, you can use "getattr" with a default to avoid this. e.g.: getattr(ssl, "OPENSSL_VERSION_NUMBER", None) Feel free to ignore since this was pre-existing. http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@132 PS3, Line 132: @pytest.mark.skipif(HAS_TLSV12_INCOMPATIBLE_PYTHON, reason=SKIP_TLSV12_MSG) Consider just inlining @pytest.mark.skipif(sys.version_info < REQUIRED_MIN_PYTHON_VERSION_FOR_TLSV12, reason="Python version too old to allow Thrift client to use TLSv1.2") -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Wed, 30 May 2018 18:51:32 +0000 Gerrit-HasComments: Yes
