David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15345 )
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks ...................................................................... Patch Set 4: Code-Review+1 (2 comments) Sorry for the delay Tamas. I think this is much more elegant that what we had before. I'll give a final +2 after a couple of small nits are fixed. http://gerrit.cloudera.org:8080/#/c/15345/4/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15345/4/shell/impala_shell.py@249 PS4, Line 249: msg = "Unable to load command history (disabling history collection): %s" % i I think reading the history file in the body of the __init__ method makes a lot more sense. This is a good solution. I have just a few quick nits. The first is, can we prepend this line with "WARNING: " as we do in the case of a ValueError exception? http://gerrit.cloudera.org:8080/#/c/15345/4/shell/impala_shell.py@254 PS4, Line 254: self._disable_readline() Can you add an error message in case of an ImportError as well? Something like: "WARNING: Unable to import readline module -- disabling impala-shell command history." -- To view, visit http://gerrit.cloudera.org:8080/15345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c Gerrit-Change-Number: 15345 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Vincent Tran <vtt...@cloudera.com> Gerrit-Comment-Date: Wed, 22 Apr 2020 16:36:30 +0000 Gerrit-HasComments: Yes
