impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
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. (

Change subject: IMPALA-2235: Fix current db when shell auto-reconnects

Patch Set 2:


Thanks for tackling this! As a user, I hate that this was broken.

I think your changes to look good. I'm a bit more wary of having another superclass
for tests with helper functions.
File shell/
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?
File tests/common/
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?
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.
File tests/custom_cluster/
PS2, Line 25: TMP_HISTORY_FILE = os.path.expanduser("~/.impalahistorytmp")
Perhaps use an actual tempfile created with tempfile.[something]
PS2, Line 27: class TestShellInteractiveReconnect(ClusterController):
Any reason not to add this simply to tests/shell/

I think because you need CustomCluster to do the restart? If so, please add a comment about
PS2, Line 40:   @pytest.mark.execute_serially
I think all CustomCluster tests run serially? This one seems like it could be parallel.
File tests/shell/
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Thu, 26 Oct 2017 17:58:48 +0000
Gerrit-HasComments: Yes

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