Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 )
Change subject: [WIP] IMPALA-7555: Impala-shell should timeout if it fails to connect ...................................................................... Patch Set 1: (17 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: should Rephrase to state what the change does, not what it should do, e.g. "Set socket timeout in impala-shell" 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. 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 http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@254 PS1, Line 254: ret = self._get_socket_and_transport() : sock, self.transport = ret[0], ret[1] You can write this in a single line: sock, self.transport = self._get_socket_and_transport() http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@257 PS1, Line 257: self.transport.open() : protocol = TBinaryProtocol.TBinaryProtocol(self.transport) : self.imp_service = ImpalaService.Client(protocol) : result = self.ping_impala_service() Which of these 4 lines can time out? Which of the ones that can time out do we currently test? There doesn't seem to be a test that connects successfully but times out in the ssl connection setup. http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@261 PS1, Line 261: sock.setTimeout(None) This could be more clear if we pass 0 instead http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@274 PS1, Line 274: """Create a Transport. update the comment http://gerrit.cloudera.org:8080/#/c/11540/1/shell/impala_client.py@326 PS1, Line 326: return TSaslClientTransport(sasl_factory, "GSSAPI", sock) This will now return a tuple in some cases, and a transport in others. Please make it consistent so that the function returns the same types in all cases. 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 host during startup 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': 60000, How did you decide on 60s? Why not pick something smaller, e.g. 5s? 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? 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: Test nit: Tests http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@743 PS1, Line 743: s = socket.socket() Use contextlib.closing here: https://docs.python.org/2/library/contextlib.html#contextlib.closing http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@745 PS1, Line 745: 1 Please add a comment what this 1 does? http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@746 PS1, Line 746: impalad test_port? It's not an impalad http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@747 PS1, Line 747: r > flake8: E501 line too long (94 > 90 characters) use ' and " consistently in this line http://gerrit.cloudera.org:8080/#/c/11540/1/tests/shell/test_shell_commandline.py@747 PS1, Line 747: 60000 This means that the test takes 60s to execute, right? Can you pick a smaller value instead, e.g. 2s? -- 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: 1 Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Fri, 28 Sep 2018 21:21:27 +0000 Gerrit-HasComments: Yes
