Fang-Yu Rao 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: > Patch Set 4: Code-Review+1 > > > Patch Set 4: > > > > Thank you for the detailed review. > > > > While I was thinking on the bookkeeping idea I could not find any other > > occurrence of calling the preloop() other than the ImpalaShell's base class > > Cmd, which has the cmdloop() method that is: > > Repeatedly issue a prompt, accept input, parse an initial prefix off the > > received input, and dispatch to action methods, passing them the remainder > > of the line as argument. > > > > While ImpalaShell is alive Cmd.cmdloop() is re-started by ImpalaShell here: > > https://github.com/apache/impala/blob/master/shell/impala_shell.py#L1882 > > Every time the cmdloop() is called it calls preloop() as well, this could > > be due to different exceptions in the cmdloop(): > > > > postloop() saves the content of the history however it is not called when > > an exception occurs. > > > > Given these conditions, I decided to refactor the history reading logic and > > moved outside of the preloop() method, so it will only be called once per > > ImpalaShell object. Let me know your thoughts. > > Thanks Tamas for your detailed analysis! > > I think I understand the root cause of this issue more after your explanation. > > When there is no exception of KeyboardInterrupt, the function of postloop() > overridden in impala_shell.py will write those items currently in the history > to the history file and thus get_current_history_length() (the number of > items currently in the history) will become 0 after this. Therefore, it is > okay for preloop() overridden in impala_shell.py to load the history from > file the next time shell.cmdloop() is called since there is no item currently > in the history and hence there will be no duplicate item in this case. > > But when there is an exception of KeyboardInterrupt caught, postloop() is not > called so that the number of items currently in the history is not 0, i.e., > get_current_history_length() is not equal to 0. Therefore, the next time when > shell.cmdloop() is called, the function preloop() will be called, resulting > in those items in the history file being loaded to the currently non-empty > history maintained by self.readline(). > > Is my understanding correct? If so, then I do not have any other comment. It > seems that your new solution is more elegant than your previous approach. :-) I just realized my understanding is not completely correct. I found in my dev environment that after readline.write_history_file() in postloop(), the number of items currently in the history will not become 0. But if this is the case, then I do not understand why in the case when there is no exception of KeyboardInterrupt, there will be no duplicate item. Maybe I miss something. -- 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: Tue, 07 Apr 2020 18:53:43 +0000 Gerrit-HasComments: No
