This is an automated email from the ASF dual-hosted git repository. joemcdonnell pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit bad064dbeab8a8e7f73c1f0ac5abe0c33f1ca967 Author: Joe McDonnell <[email protected]> AuthorDate: Fri Jun 16 16:10:29 2023 -0700 IMPALA-12224: Improve error handling for shell interactive tests Interactive shell tests can hang waiting for input if the shell process hits errors or exits. For example, the problems in the sasl package seen in IMPALA-12220 cause test_shell_interactive.py to hang. This improves the error detection/handling to avoid hangs for most common shell errors. Specifically, it adds a check for the impala-shell process exiting, and it adds a check for a failure to connect to Impala. Both would previous result in hangs. Testing: - Verified test_shell_interactive.py doesn't hang with hand tests - Remove a vital import from impala-shell so it exits instantly - Simulate a connection problem by overwriting the port with a non-functional port - Test on Redhat 9 with the IMPALA-12220 issue Change-Id: I7556fb687e06b41caa538d8c3231ec9f2ad98162 Reviewed-on: http://gerrit.cloudera.org:8080/20087 Reviewed-by: Michael Smith <[email protected]> Tested-by: Joe McDonnell <[email protected]> --- shell/impala_shell.py | 26 +++++++++++++++++++------- tests/shell/util.py | 41 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/shell/impala_shell.py b/shell/impala_shell.py index a0896f916..919aea24b 100755 --- a/shell/impala_shell.py +++ b/shell/impala_shell.py @@ -137,14 +137,23 @@ class ImpalaShell(cmd.Cmd, object): Status tells the caller that the command completed successfully. """ + # NOTE: These variables are centrally defined for reuse, but they are also + # used directly in several tests to verify shell behavior (e.g. to + # verify that the shell remained connected or the shell connected + # successfully). + # If not connected to an impalad, the server version is unknown. UNKNOWN_SERVER_VERSION = "Not Connected" PROMPT_FORMAT = "[{host}:{port}] {db}> " DISCONNECTED_PROMPT = "[Not connected] > " # Message to display when the connection failed and it is reconnecting. - # This is used in some tests for verification that the session remained - # connected. CONNECTION_LOST_MESSAGE = 'Connection lost, reconnecting...' + # Message to display when there is an exception when connecting. + ERROR_CONNECTING_MESSAGE = "Error connecting" + # Message to display when there is a socket error. + SOCKET_ERROR_MESSAGE = "Socket error" + # Message to display upon successful connection to an Impalad + CONNECTED_TO_MESSAGE = "Connected to" # Message to display in shell when cancelling a query CANCELLATION_MESSAGE = ' Cancelling Query' # Number of times to attempt cancellation before giving up. @@ -1015,7 +1024,8 @@ class ImpalaShell(cmd.Cmd, object): pass if self.imp_client.connected: - self._print_if_verbose('Connected to %s:%s' % self.impalad) + self._print_if_verbose('%s %s:%s' % + (self.CONNECTED_TO_MESSAGE, self.impalad[0], self.impalad[1])) self._print_if_verbose('Server version: %s' % self.server_version) self.set_prompt(ImpalaShell.DEFAULT_DB) self._validate_database() @@ -1084,7 +1094,7 @@ class ImpalaShell(cmd.Cmd, object): if e.errno == errno.EINTR: self._reconnect_cancellation() else: - print("Socket error %s: %s" % (e.errno, e), file=sys.stderr) + print("%s %s: %s" % (self.SOCKET_ERROR_MESSAGE, e.errno, e), file=sys.stderr) self.close_connection() self.prompt = self.DISCONNECTED_PROMPT except Exception as e: @@ -1096,7 +1106,8 @@ class ImpalaShell(cmd.Cmd, object): if self.use_ssl and sys.version_info < (2,7,9) \ and "EOF occurred in violation of protocol" in str(e): print("Warning: TLSv1.2 is not supported for Python < 2.7.9", file=sys.stderr) - print("Error connecting: %s, %s" % (type(e).__name__, e), file=sys.stderr) + print("%s: %s, %s" % + (self.ERROR_CONNECTING_MESSAGE, type(e).__name__, e), file=sys.stderr) # A secure connection may still be open. So we explicitly close it. self.close_connection() # If a connection to another impalad failed while already connected @@ -1452,7 +1463,7 @@ class ImpalaShell(cmd.Cmd, object): print(ImpalaShell.CANCELLATION_MESSAGE) self._reconnect_cancellation() else: - print("Socket error %s: %s" % (e.errno, e), file=sys.stderr) + print("%s %s: %s" % (self.SOCKET_ERROR_MESSAGE, e.errno, e), file=sys.stderr) self.prompt = self.DISCONNECTED_PROMPT self.imp_client.connected = False except Exception as e: @@ -2269,7 +2280,8 @@ def impala_shell_main(): print(shell.CANCELLATION_MESSAGE) shell._reconnect_cancellation() else: - print("Socket error %s: %s" % (e.errno, e), file=sys.stderr) + print("%s %s: %s" % + (shell.SOCKET_ERROR_MESSAGE, e.errno, e), file=sys.stderr) shell.imp_client.connected = False shell.prompt = shell.DISCONNECTED_PROMPT except DisconnectedException as e: diff --git a/tests/shell/util.py b/tests/shell/util.py index fe30b83a7..06f62842d 100755 --- a/tests/shell/util.py +++ b/tests/shell/util.py @@ -32,6 +32,12 @@ import sys import time from subprocess import Popen, PIPE +# This import is the actual ImpalaShell class from impala_shell.py. +# We rename it to ImpalaShellClass here because we later import another +# class called ImpalaShell from tests/shell/util.py, and we don't want +# to mask it. +from shell.impala_shell import ImpalaShell as ImpalaShellClass + from tests.common.environ import (IMPALA_LOCAL_BUILD_VERSION, ImpalaTestClusterProperties) from tests.common.impala_service import ImpaladService @@ -229,10 +235,41 @@ class ImpalaShell(object): # if stderr is redirected. if wait_until_connected and (args is None or "--quiet" not in args) and \ stderr_file is None: + # We don't want to hang waiting for input. So, here are the scenarios + # we need to handle: + # 1. Shell process exits + # 2. Shell fails to connect. This can lead to an interactive prompt + # that blocks for input forever. The two messages to look for are + # "Error connecting" and "Socket error" + # 3. Process successfully connecting: "Connected to" + # Cases 1 and 2 should lead to an assert. start_time = time.time() + process_status = None + connection_err = None connected = False - while time.time() - start_time < timeout and not connected: - connected = "Connected to" in self.shell_process.stderr.readline() + while time.time() - start_time < timeout: + # Condition 1: check if the shell process has exited + # poll() returns None until the process exits + process_status = self.shell_process.poll() + if process_status is not None: + break + # readline() can block forever, so the timeout logic may not be effective + # if something gets stuck here. + line = self.shell_process.stderr.readline() + # Condition 2: check for errors connecting + if ImpalaShellClass.ERROR_CONNECTING_MESSAGE in line or \ + ImpalaShellClass.SOCKET_ERROR_MESSAGE in line: + connection_err = line + break + + # Condition 3: check if the connection is successful + connected = ImpalaShellClass.CONNECTED_TO_MESSAGE in line + if connected: + break + + assert process_status is None, \ + "Impala shell exited with return code {0}".format(process_status) + assert connection_err is None, connection_err assert connected, "Impala shell is not connected" def pid(self):
