impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Fang-Yu Rao (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Date Tue, 07 Apr 2020 17:52:26 GMT
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:
> 
> 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. :-)


-- 
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 <tmate@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fangyu.rao@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tmate@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vttran@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 17:52:26 +0000
Gerrit-HasComments: No

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