Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/10992 )
Change subject: IMPALA-6490: Reconnect shell when remote restarts ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/10992/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10992/2//COMMIT_MSG@12 PS2, Line 12: noew now http://gerrit.cloudera.org:8080/#/c/10992/2/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/10992/2/shell/impala_client.py@33 PS2, Line 33: from thrift.Thrift import TApplicationException : from thrift.Thrift import TException nit: One line http://gerrit.cloudera.org:8080/#/c/10992/2/shell/impala_client.py@234 PS2, Line 234: "Checks to see if the current Impala connection is still alive. If not, an exception : will be raised. Comment is still incorrect. http://gerrit.cloudera.org:8080/#/c/10992/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/10992/2/shell/impala_shell.py@566 PS2, Line 566: test_connection test_connected() could be renamed to is_connected() or something similar to make clear that we expect a True/False return value. http://gerrit.cloudera.org:8080/#/c/10992/2/shell/impala_shell.py@566 PS2, Line 566: connected = self.imp_client.test_connection() : if not connected: Introducing 'connected' is unnecessary. Mathod should be called in the if statement. http://gerrit.cloudera.org:8080/#/c/10992/2/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/10992/2/tests/custom_cluster/test_shell_interactive_reconnect.py@84 PS2, Line 84: proc = pexpect.spawn(SHELL_CMD) Would it be possible to use ImpalaShell() object here instead of pexpect? (just like in 'test_auto_reconnect' above). 'ImpalaShell' class also has a 'prompt' member that we could use for this test (instead of pexpect.expect()). http://gerrit.cloudera.org:8080/#/c/10992/2/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/10992/2/tests/shell/test_shell_interactive.py@43 PS2, Line 43: START_CLUSTER = "%s/bin/start-impala-cluster.py" % os.environ['IMPALA_HOME'] 'START_CLUSTER' is not used anywhere, you can remove this line. -- To view, visit http://gerrit.cloudera.org:8080/10992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia13365a9696886f01294e98054cf4e7cd66ab712 Gerrit-Change-Number: 10992 Gerrit-PatchSet: 2 Gerrit-Owner: Le Minh Nghia <le.ng...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Le Minh Nghia <le.ng...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 20 Jul 2018 10:35:11 +0000 Gerrit-HasComments: Yes
