You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2020/03/03 07:50:54 UTC
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Tamas Mate has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15345
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks
......................................................................
IMPALA-9398: Fix shell history duplication when cmdloop breaks
This change adds a new condition to avoid re-reading the impala-shell
history when the cmdloop is broken. The loop can break due to exceptions
such as KeyboardInterrupt.
Testing:
- The change was testad manually on my local dev env
- Added a new EE shell test to verify the history after SIGINT
Change-Id: If4faf46134f44d91e56748642f47d448707db53c
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 36 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/15345/1
--
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: newchange
Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Gerrit-Change-Number: 15345
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/15345 )
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks
......................................................................
IMPALA-9398: Fix shell history duplication when cmdloop breaks
This change adds a new condition to avoid re-reading the impala-shell
history when the cmdloop is broken. The loop can break due to exceptions
such as KeyboardInterrupt.
Testing:
- The change was tested manually on local dev env
- Added a new EE shell test to verify the history after SIGINT
Change-Id: If4faf46134f44d91e56748642f47d448707db53c
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 39 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/15345/2
--
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: newpatchset
Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Gerrit-Change-Number: 15345
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
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 1:
(2 comments)
Thanks for the fixing this problem! I only have two very minor comments. Thanks!
http://gerrit.cloudera.org:8080/#/c/15345/1/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/15345/1/shell/impala_shell.py@1366
PS1, Line 1366: if self.readline.get_current_history_length() == 0:
> It's not altogether clear what the significance of current history length b
Is it true that when the Impala shell is running and there is an interrupt signal caught, the exception will be caught at https://github.com/apache/impala/blob/master/shell/impala_shell.py#L1859. But since the shell is still alive (https://github.com/apache/impala/blob/master/shell/impala_shell.py#L1855), so shell.cmdloop() will be called again. Is my understanding correct? Thanks!
http://gerrit.cloudera.org:8080/#/c/15345/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:
http://gerrit.cloudera.org:8080/#/c/15345/1/tests/shell/test_shell_interactive.py@473
PS1, Line 473: p = ImpalaShell(vector)
Is it true that the reason we use ImpalaShell() here instead of pexpect.spawn() to construct a new instance is because we have splitlines() that helps us splitting the returned lines when ImpalaShell()? Thanks!
--
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: 1
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 19:25:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 01:50:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
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 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15345/1/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/15345/1/shell/impala_shell.py@1366
PS1, Line 1366: if self.readline.get_current_history_length() == 0:
It's not altogether clear what the significance of current history length being equal to 0 on this line means in relation to having issued a SIGINT. Can you elaborate with a code comment here?
Without looking through the surrounding code myself, my guess is that this is a method that might be re-entered several times during a running shell session, and that the history length will only be 0 the first time, right after the shell is first started. For any subsequent invocation of this method, some history will have accumulated, so this check will fail.
Unfortunately, as there's nothing in this method's docstring or other comments to indicate that this method might be called multiple times, this remains just speculation.
--
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: 1
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 17:52:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5497/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Mar 2020 08:07:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5: Verified+1
--
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: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 16:08:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5706/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Apr 2020 09:21:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/15345 )
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks
......................................................................
IMPALA-9398: Fix shell history duplication when cmdloop breaks
The duplication issue was due to calling the history file reading
readline method multiple times during the lifetime of an ImpalaShell.
Readline appends the content of the file after the already read history.
History was read in the Cmd.preloop() method which is called every time
when the Cmd.cmdloop() starts, the cmdloop() break and re-start could be
triggered by an exception for example KeyboardInterrupt in this case.
This change refactors the history file reading logic and moves it
outside of the preloop() method.
Testing:
- The change was tested manually on local dev env
- Added a new EE shell test to verify the history after SIGINT
Change-Id: If4faf46134f44d91e56748642f47d448707db53c
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 43 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/15345/4
--
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: newpatchset
Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Gerrit-Change-Number: 15345
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
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:
> I do not have any other comment on this patch now.
I'll give it another look this evening or tomorrow A.M.
--
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 22:24:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 19:03:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Vincent Tran 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 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15345/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15345/1//COMMIT_MSG@14
PS1, Line 14: testad
nit: spelling
http://gerrit.cloudera.org:8080/#/c/15345/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:
http://gerrit.cloudera.org:8080/#/c/15345/1/tests/shell/test_shell_interactive.py@455
PS1, Line 455: set up history
Should the setup include zero'ing out ~/.impalahistory? Otherwise, it could become flaky if other tests happen to use impala-shell and alter the first few entries.
--
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: 1
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 21:09:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has removed a vote on this change.
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks
......................................................................
Removed Code-Review+1 by Fang-Yu Rao <fa...@cloudera.com>
--
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: deleteVote
Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Gerrit-Change-Number: 15345
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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: Verified+1
--
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Apr 2020 13:54:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 17:52:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15345 )
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks
......................................................................
IMPALA-9398: Fix shell history duplication when cmdloop breaks
This change adds a new condition to avoid re-reading the impala-shell
history when the cmdloop is broken. The loop can break due to exceptions
such as KeyboardInterrupt.
Testing:
- The change was tested manually on local dev env
- Added a new EE shell test to verify the history after SIGINT
Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Reviewed-on: http://gerrit.cloudera.org:8080/15345
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 48 insertions(+), 17 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Gerrit-Change-Number: 15345
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate 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:
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.
--
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 09:18:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Code-Review+2
--
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: 6
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 20:42:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15345/1/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/15345/1/shell/impala_shell.py@1366
PS1, Line 1366: try:
> Thanks Tamas! After your explanation, I think I understand why your patch f
I actually think the bookkeeping idea is not a bad idea, but do we know if there are any other conditions that might lead us back into the preloop() method?
--
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: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Mar 2020 18:15:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate 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:
(2 comments)
Thank you for the review David, Fang-Yu.
Answered the last couple of comments, let me know if I should add anything else.
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
Sure, looks better this way.
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 l
Sure, looks better this way.
--
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 10:39:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified+1
--
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: 6
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Apr 2020 01:55:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Apr 2020 16:36:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5722/ DRY_RUN=true
--
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: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 10:51:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5399/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 08:33:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
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 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15345/1/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:
http://gerrit.cloudera.org:8080/#/c/15345/1/tests/shell/test_shell_interactive.py@473
PS1, Line 473: p = ImpalaShell(vector)
> Is it true that the reason we use ImpalaShell() here instead of pexpect.spa
Sorry there were two missing words in my previous question. The following is my revised question.
Is it true that the reason we use ImpalaShell() here instead of pexpect.spawn() to construct a new instance is because we have splitlines() that helps us splitting the returned lines when ImpalaShell() is used? Thanks!
--
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: 1
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 19:28:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 18:53:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5612/ DRY_RUN=true
--
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 <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Apr 2020 09:27:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 20:42:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2: Verified+1
--
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: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Mar 2020 22:45:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5488/ DRY_RUN=true
--
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: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Mar 2020 17:49:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/15345 )
Change subject: IMPALA-9398: Fix shell history duplication when cmdloop breaks
......................................................................
IMPALA-9398: Fix shell history duplication when cmdloop breaks
This change adds a new condition to avoid re-reading the impala-shell
history when the cmdloop is broken. The loop can break due to exceptions
such as KeyboardInterrupt.
Testing:
- The change was tested manually on local dev env
- Added a new EE shell test to verify the history after SIGINT
Change-Id: If4faf46134f44d91e56748642f47d448707db53c
---
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
2 files changed, 48 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/15345/5
--
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: newpatchset
Gerrit-Change-Id: If4faf46134f44d91e56748642f47d448707db53c
Gerrit-Change-Number: 15345
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5909/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 11:24:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9398: Fix shell history duplication when cmdloop breaks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5724/ DRY_RUN=false
--
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: 6
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Apr 2020 20:42:56 +0000
Gerrit-HasComments: No