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 <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 13 Nov 2017 19:14:30 +0000 Gerrit-HasComments: Yes
