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: Code-Review+1 > 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. I think in the case when there is no exception of KeyboardInterrupt, the function preloop() overridden in implala_shell.py will only be executed once and hence there will be no duplicate item in the current history. And I think it may be a good idea to move the loading of history to the constructor of ImpalaShell so that we could guarantee that read_history_file() is only done once. -- 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 19:03:07 +0000 Gerrit-HasComments: No
