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: > > Hi Fang-Yu, > > It is a bit hard to create formatted text on gerrit, I am splitting the > paragraphs with "---". > --- > ImpalaShell.postloop() will only be called once per ImpalaShell object > lifetime because it is tied to ImpalaShell.is_alive. The postloop() does the > persisting of history and can be reached when the Cmd.cmdloop() is stopped > normally. This is where it is called: > https://github.com/python/cpython/blob/master/Lib/cmd.py#L139 > --- > This postloop() can be reached when the postcmd() in line 139 returns True. > ImpalaShell.postcmd() just returns the status provided by the caller. > https://github.com/apache/impala/blob/master/shell/impala_shell.py#L664 > --- > The status can be the following three predefined in impala-shell.py CmdStatus: > https://github.com/apache/impala/blob/master/shell/impala_shell.py#L81 > --- > I am skipping some part of the code flow here and changing my approach. From > this point the relevant status is CmdStatus.ABORT, because that is the only > one with True value which could stop the Cmd.cmdloop() normally. Abort is > only returned by the ImpalaShell.do_quit(). This method can be called > multiple ways, however it ties the ABORT and is_alive=False together. When > is_alive is false, ImpalaShell's main loop terminates, therefore with normal > quit both loops and ImpalaShell will be terminated. > https://github.com/apache/impala/blob/master/shell/impala_shell.py#L787 > --- > The reading logic was moved to the constructor next to where the readline > module is being imported. Thanks Tamas for your detailed explanation! Using a debugger I am able to understand what happen after a user presses Ctrl-D (EOF) in the Impala shell. 1. Given an EOF, ImpalaShell.precmd() returns "quit" (https://github.com/python/cpython/blob/master/Lib/cmd.py#L137). 2. Given "quit", ImpalaShell.onecmd() returns "CmdStatus.ABORT" (defined as True) after calling do_quit() that sets 'ImpalaShell.is_alive' to False (https://github.com/python/cpython/blob/master/Lib/cmd.py#L138). 3. Given "CmdStatus.ABORT", ImpalaShell.postcmd() returns "CmdStatus.ABORT", which is assigned to the variable 'stop' (https://github.com/python/cpython/blob/master/Lib/cmd.py#L139). 4. Since 'stop' now evaluates to True, we leave the while-loop (https://github.com/python/cpython/blob/master/Lib/cmd.py#L120) and then ImpalaShell.postloop() is executed (https://github.com/python/cpython/blob/master/Lib/cmd.py#L140). I do not have any other comment on this patch now. -- 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: Thu, 09 Apr 2020 01:50:19 +0000 Gerrit-HasComments: No
