Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/10992 )
Change subject: IMPALA-6490: Reconnect shell when remote restarts ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/10992/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10992/5//COMMIT_MSG@11 PS5, Line 11: need nit: "needed" would be better http://gerrit.cloudera.org:8080/#/c/10992/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/10992/5/shell/impala_shell.py@614 PS5, Line 614: # After reconnecting, the prompt name should also be updated nit: . at the end http://gerrit.cloudera.org:8080/#/c/10992/5/shell/impala_shell.py@862 PS5, Line 862: if self.imp_client.connected: It is a matter of taste, but I would change this to if not self.imp_client.connected: return to reduce nesting http://gerrit.cloudera.org:8080/#/c/10992/5/shell/impala_shell.py@863 PS5, Line 863: # Should only check if succesfully connected nit: . at the end http://gerrit.cloudera.org:8080/#/c/10992/5/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/10992/5/tests/custom_cluster/test_shell_interactive_reconnect.py@89 PS5, Line 89: self._start_impala_cluster(["--kill"]) I would prefer to kill/restart only the impalad that the shell has connected to. The reason is that restarting the whole cluster takes much more time. Another speed related improvement could be to use another impalad than the default here - as impalad caches metadata, restarting it can make other tests slower (I guess that most tests connect to the default impalad, but I did not look into this deeply). -- 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: 5 Gerrit-Owner: Le Minh Nghia <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Le Minh Nghia <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 23 Jul 2018 11:04:56 +0000 Gerrit-HasComments: Yes
