anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 )
Change subject: [WIP] IMPALA-7555: Set socket timeout in impala-shell ...................................................................... Patch Set 3: (18 comments) http://gerrit.cloudera.org:8080/#/c/11540/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11540/1//COMMIT_MSG@7 PS1, Line 7: meout > Rephrase to state what the change does, not what it should do, e.g. "Set so Done http://gerrit.cloudera.org:8080/#/c/11540/1//COMMIT_MSG@19 PS1, Line 19: 2. Created a kerberized impala cluster with ssl enabled and connected > This should be automated, too. 1. I am not sure if it's easy to automate this test on a minicluster setup (kerberos + SSL) 2. I ran test_client_ssl.py and created a minicluster setup with SSL enabled. Tried to block the beeswax server with telnet or with an openssl client, but it did not block the beeswax server (with just SSL enabled.) http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@81 PS1, Line 81: client_connect_timeout > Is this milliseconds? If so, please rename to client_connect_timeout_ms Done http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@254 PS1, Line 254: sock, self.transport = self._get_socket_and_transport() : sock.setTimeout(self.client_connect_t > You can write this in a single line: Done http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@257 PS1, Line 257: protocol = TBinaryProtocol.TBinaryProtocol(self.transport) : self.imp_service = ImpalaService.Client(protocol) : result = self.ping_impala_service() : sock.setTimeout(None) > Which of these 4 lines can time out? Which of the ones that can time out do Let me get back to you shortly on this one. http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@261 PS1, Line 261: self.connected = True > This could be more clear if we pass 0 instead Does not work as expected if we pass 0 Here is the definition in - build/impala-shell-3.1.0-SNAPSHOT/lib/thrift/transport/TSocket.py def setTimeout(self, ms): if ms is None: self.__timeout = None else: self.__timeout = ms / 1000.0 http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@274 PS1, Line 274: > update the comment Updated the comment to mention what this function returns. http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@326 PS1, Line 326: return sock, TSaslClientTransport(sasl_factory, "GSSAPI", sock) > This will now return a tuple in some cases, and a transport in others. Plea oops. I missed returning from this inner function. Let me make sure I test this part too. http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_shell.py@864 PS1, Line 864: self.imp_client.close_connection() > Please add a test that calls connect from the shell instead of passing a ho Working on it. Will send a new patch with this change. http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_shell_config_defaults.py File shell/impala_shell_config_defaults.py: http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_shell_config_defaults.py@54 PS1, Line 54: 'client_connect_timeout_ms': 5000, > How did you decide on 60s? Why not pick something smaller, e.g. 5s? I tried to keep this value large by default for secure connections to establish. I searched around a bit after reading your comment and I think 5 secs should suffice for a connection to be established. Do you know of any tests to check if 5secs would be good enough? http://gerrit.cloudera.org:8080/#/c/11540/1/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/11540/1/shell/option_parser.py@225 PS1, Line 225: client_connect_timeout > client_connect_timeout_ms? Done http://gerrit.cloudera.org:8080/#/c/11540/1/shell/option_parser.py@226 PS1, Line 226: " > flake8: E251 unexpected spaces around keyword / parameter equals Done http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@737 PS1, Line 737: est_ > nit: Tests Done http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@743 PS1, Line 743: """ > Use contextlib.closing here: https://docs.python.org/2/library/contextlib.h Done http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@744 PS1, Line 744: n > flake8: E231 missing whitespace after ',' Done http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@745 PS1, Line 745: ( > Please add a comment what this 1 does? Done http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@746 PS1, Line 746: aximum > test_port? It's not an impalad Done http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@747 PS1, Line 747: > use ' and " consistently in this line Done -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 3 Gerrit-Owner: anujphadke <apha...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Mon, 01 Oct 2018 08:28:27 +0000 Gerrit-HasComments: Yes
