impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Borok-Nagy (Code Review)" <ger...@cloudera.org>
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. ( http://gerrit.cloudera.org:8080/8368
)

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


Patch Set 4:

(3 comments)

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.

http://gerrit.cloudera.org:8080/#/c/8368/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8368/3//COMMIT_MSG@9
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.


http://gerrit.cloudera.org:8080/#/c/8368/3/tests/custom_cluster/test_shell_interactive_reconnect.py
File tests/custom_cluster/test_shell_interactive_reconnect.py:

http://gerrit.cloudera.org:8080/#/c/8368/3/tests/custom_cluster/test_shell_interactive_reconnect.py@36
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:
https://docs.python.org/2/library/tempfile.html#tempfile.mktemp

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.
Done.


http://gerrit.cloudera.org:8080/#/c/8368/3/tests/shell/util.py
File tests/shell/util.py:

http://gerrit.cloudera.org:8080/#/c/8368/3/tests/shell/util.py@121
PS3, Line 121: def restore_shell_history(filepath):
> mention that this is a no-op if filepath doesn't exist.
Done.



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

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