Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8368 )
Change subject: IMPALA-2235: Fix current db when shell auto-reconnects ...................................................................... Patch Set 2: (7 comments) Thanks for tackling this! As a user, I hate that this was broken. I think your changes to impala_shell.py look good. I'm a bit more wary of having another superclass for tests with helper functions. http://gerrit.cloudera.org:8080/#/c/8368/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8368/2/shell/impala_shell.py@729 PS2, Line 729: def _validate_database(self, immediately=False): Is there a case where you ever want immediately=True? i.e., could we get rid of the two paths here and converge to just one? http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py File tests/common/cluster_controller.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@33 PS2, Line 33: class ClusterController(CustomClusterTestSuite): I'm generally not fond of the inheritance and multiple-inheritance in these tests. Is this actually distinct from CustomClusterTestSuite? http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@64 PS2, Line 64: def start_cluster_with_args(self, **kwargs): This could go straight into the super class. It's tightly coupled with _start_impala_cluster. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@25 PS2, Line 25: TMP_HISTORY_FILE = os.path.expanduser("~/.impalahistorytmp") Perhaps use an actual tempfile created with tempfile.[something] http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@27 PS2, Line 27: class TestShellInteractiveReconnect(ClusterController): Any reason not to add this simply to tests/shell/test_shell_interactive.py? I think because you need CustomCluster to do the restart? If so, please add a comment about this. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@40 PS2, Line 40: @pytest.mark.execute_serially I think all CustomCluster tests run serially? This one seems like it could be parallel. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/shell/util.py File tests/shell/util.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/shell/util.py@116 PS2, Line 116: """ Moves history file to given filepath """ The underlying bug here is that the shell doesn't have a historypath option, but I think this is fine... -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 26 Oct 2017 17:58:48 +0000 Gerrit-HasComments: Yes
