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': 6,
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: 6
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
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Comment-Date: Fri, 28 Sep 2018 21:21:27 +
Gerrit-HasComments: Yes
