impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Date Thu, 26 Oct 2017 17:58:48 GMT
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 <boroknagyz@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 17:58:48 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message