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 Mon, 13 Nov 2017 19:14:30 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 3: Code-Review+1

(3 comments)

Thanks for the updates. This looks fine to me, with 2 minor issues:

* Please re-work the first line of the commit message; it doesn't make sense to me. (Or maybe
I'm missing something, in which case just let me know.)

* If you're using NamedTemporaryFile to just get a filename, I suggested a shorter solution.
There are two cases in this diff where you could take advantage of that.

I'm fine with both of those changes going in without further review; they're very minor.

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: When precmd tested the connection it didn't validate that if we are
This sentence didn't parse for me. Specifically, I'm not sure what "that" in "it didn't validate
that" is referring to.


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:     tempfile = NamedTemporaryFile(delete=False)
            :     tempfile.close()
            :     cls.tempfile_name = tempfile.name
Minor: I think this is just:

cls.tempfile_name = tempfile.mktemp()

Or did you use NamedTemporaryFile to create the file too, and that's important somehow?


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:   """ Moves back history file from given filepath """
mention that this is a no-op if filepath doesn't exist.



-- 
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: 3
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: Mon, 13 Nov 2017 19:14:30 +0000
Gerrit-HasComments: Yes

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