impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Borok-Nagy (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Date Tue, 14 Nov 2017 15:34:03 GMT
Zoltan Borok-Nagy has posted comments on this change. (

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

Patch Set 4:


Thanks for the review.

In addition I also added a time.sleep(1) to the test case test_auto_reconnect because I noticed
that it wasn't stable.
This sleep gives time for the ImpalaShell to successfully start and connect to the Impala
daemon before we restart the cluster.
Commit Message:
PS3, Line 9: The ImpalaShell didn't issue the 'USE <current-db>' command after
> This sentence didn't parse for me. Specifically, I'm not sure what "that" i
I rephrased the beginning of the commit. I hope it is easier to understand now.
File tests/custom_cluster/
PS3, Line 36: 
            :     cls.tempfile_name = tempfile.mktemp()
            :     move_shell_history(cls.tempfile_n
> Minor: I think this is just:
I only used NamedTemporaryFile because mktemp is marked as deprecated:

tempfile.mkstemp() could have also been used, but it also creates a file, and returns an open
file descriptor for that.

Anyway, since it is in a test case, I don't see that mktemp() would cause a vulnerability
issue, so now I use it for brevity and better readability.
File tests/shell/
PS3, Line 121: def restore_shell_history(filepath):
> mention that this is a no-op if filepath doesn't exist.

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: 4
Gerrit-Owner: Zoltan Borok-Nagy <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Tue, 14 Nov 2017 15:34:03 +0000
Gerrit-HasComments: Yes

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