[impala] branch master updated: IMPALA-9398: Fix shell history duplication when cmdloop breaks

[ta...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git


The following commit(s) were added to refs/heads/master by this push:
     new 1a36a03  IMPALA-9398: Fix shell history duplication when cmdloop breaks
1a36a03 is described below

commit 1a36a0348b85c2b2008d7b4b3a061a467f8456e7
Author: Tamas Mate <tm...@cloudera.com>
AuthorDate: Mon Mar 2 16:13:53 2020 +0100

    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>
---
 shell/impala_shell.py                 | 32 +++++++++++++++-----------------
 tests/shell/test_shell_interactive.py | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/shell/impala_shell.py b/shell/impala_shell.py
index 549cd71..8c877d1 100755
--- a/shell/impala_shell.py
+++ b/shell/impala_shell.py
@@ -230,11 +230,25 @@ class ImpalaShell(cmd.Cmd, object):
         self.readline = __import__('readline')
         try:
           self.readline.set_history_length(int(options.history_max))
+          # The history file is created when the Impala shell is invoked and commands are
+          # issued. In case it does not exist do not read the history file.
+          if os.path.exists(self.history_file):
+            self.readline.read_history_file(self.history_file)
+            self._replace_history_delimiters(ImpalaShell.HISTORY_FILE_QUERY_DELIM, '\n')
         except ValueError:
           warning = "WARNING: history_max option malformed %s\n" % options.history_max
           print(warning, file=sys.stderr)
           self.readline.set_history_length(1000)
-      except ImportError:
+        except IOError, i:
+          warning = "WARNING: Unable to load command history (disabling impala-shell " \
+              "command history): %s" % i
+          print(warning, file=sys.stderr)
+          # This history file exists but is not readable, disable readline.
+          self._disable_readline()
+      except ImportError, i:
+        warning = "WARNING: Unable to import readline module (disabling impala-shell " \
+            "command history): %s" % i
+        print(warning, file=sys.stderr)
         self._disable_readline()
 
     if options.impalad is not None:
@@ -1372,22 +1386,6 @@ class ImpalaShell(cmd.Cmd, object):
     else:
       return CmdStatus.ERROR
 
-  def preloop(self):
-    """Load the history file if it exists"""
-    if self.readline:
-      # The history file is created when the Impala shell is invoked and commands are
-      # issued. In the first invocation of the shell, the history file will not exist.
-      # Clearly, this is not an error, return.
-      if not os.path.exists(self.history_file): return
-      try:
-        self.readline.read_history_file(self.history_file)
-        self._replace_history_delimiters(ImpalaShell.HISTORY_FILE_QUERY_DELIM, '\n')
-      except IOError as i:
-        msg = "Unable to load command history (disabling history collection): %s" % i
-        print(msg, file=sys.stderr)
-        # This history file exists but is not readable, disable readline.
-        self._disable_readline()
-
   def postloop(self):
     """Save session commands in history."""
     if self.readline:
diff --git a/tests/shell/test_shell_interactive.py b/tests/shell/test_shell_interactive.py
index ceee1fd..27dc4bd 100755
--- a/tests/shell/test_shell_interactive.py
+++ b/tests/shell/test_shell_interactive.py
@@ -446,6 +446,39 @@ class TestImpalaShellInteractive(ImpalaTestSuite):
       assert history_entry in result.stderr, "'%s' not in '%s'" % (history_entry,
                                                                    result.stderr)
 
+  def test_history_does_not_duplicate_on_interrupt(self, vector, tmp_history_file):
+    """This test verifies that once the cmdloop is broken the history file will not be
+    re-read. The cmdloop can be broken when the user sends a SIGINT or exceptions
+    occur."""
+    # readline gets its input from tty, so using stdin does not work.
+    shell_cmd = get_shell_cmd(vector)
+    child_proc = pexpect.spawn(shell_cmd[0], shell_cmd[1:])
+    # set up history
+    child_proc.expect(PROMPT_REGEX)
+    child_proc.sendline("select 1;")
+    child_proc.expect("Fetched 1 row\(s\) in [0-9]+\.?[0-9]*s")
+    child_proc.expect(PROMPT_REGEX)
+    child_proc.sendline("quit;")
+    child_proc.wait()
+    child_proc = pexpect.spawn(shell_cmd[0], shell_cmd[1:])
+    child_proc.expect(PROMPT_REGEX)
+
+    # send SIGINT then quit to save history
+    child_proc.sendintr()
+    child_proc.sendline("select 2;")
+    child_proc.expect("Fetched 1 row\(s\) in [0-9]+\.?[0-9]*s")
+    child_proc.sendline("quit;")
+    child_proc.wait()
+
+    # check history in a new instance
+    p = ImpalaShell(vector)
+    p.send_cmd('history')
+    result = p.get_result().stderr.splitlines()
+    assert "[1]: select 1;" == result[1]
+    assert "[2]: quit;" == result[2]
+    assert "[3]: select 2;" == result[3]
+    assert "[4]: quit;" == result[4]
+
   def test_history_file_option(self, vector, tmp_history_file):
     """
     Setting the 'tmp_history_file' fixture above means that the IMPALA_HISTFILE