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 e9fb8e717c32a9a72dc81b85084f982efc25a2f8 Author: Joe McDonnell <[email protected]> AuthorDate: Wed May 31 13:23:52 2023 -0700 IMPALA-12114: Pull in fix for THRIFT-5705 and add test This pulls in a new toolchain to get a Thrift with the patch for THRIFT-5705. This fixes an issue where idle clients using TLS are needlessly disconnected due to a bug in the read retry count logic inside Thrift. Tests: - This modifies test_thrift_socket.py to make it do more idle polls and check that ImpalaShell is not disconnected. It fails without the THRIFT-5705 patch and passes now. Change-Id: Ifc7704cba032a91b9fd0d5d54d1e0a7e17fb10bb Reviewed-on: http://gerrit.cloudera.org:8080/19962 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Daniel Becker <[email protected]> Reviewed-by: Andrew Sherman <[email protected]> --- bin/impala-config.sh | 8 ++++---- shell/impala_shell.py | 6 +++++- tests/custom_cluster/test_thrift_socket.py | 27 +++++++++++++++++++++++---- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/bin/impala-config.sh b/bin/impala-config.sh index f07cdf4b8..478e9bf43 100755 --- a/bin/impala-config.sh +++ b/bin/impala-config.sh @@ -81,7 +81,7 @@ export USE_APACHE_HIVE=${USE_APACHE_HIVE-false} # moving to a different build of the toolchain, e.g. when a version is bumped or a # compile option is changed. The build id can be found in the output of the toolchain # build jobs, it is constructed from the build number and toolchain git hash prefix. -export IMPALA_TOOLCHAIN_BUILD_ID=268-f2a9999487 +export IMPALA_TOOLCHAIN_BUILD_ID=295-d43043e809 # Versions of toolchain dependencies. # ----------------------------------- export IMPALA_AVRO_VERSION=1.7.4-p5 @@ -184,7 +184,7 @@ unset IMPALA_CALLONCEHACK_URL # If upgrading IMPALA_THRIFT_PY_VERSION, remember to also upgrade thrift version in # shell/ext-py and shell/packaging/requirements.txt. IMPALA_THRIFT_PY_VERSION is used # with Impyla and for the thrift compiler. -export IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 +export IMPALA_THRIFT_CPP_VERSION=0.16.0-p4 unset IMPALA_THRIFT_CPP_URL if $USE_APACHE_HIVE; then # Apache Hive 3 clients can't run on thrift versions >= 0.14 (IMPALA-11801) @@ -192,10 +192,10 @@ if $USE_APACHE_HIVE; then export IMPALA_THRIFT_JAVA_VERSION=${IMPALA_THRIFT_POM_VERSION}-p5 else export IMPALA_THRIFT_POM_VERSION=0.16.0 - export IMPALA_THRIFT_JAVA_VERSION=${IMPALA_THRIFT_POM_VERSION}-p3 + export IMPALA_THRIFT_JAVA_VERSION=${IMPALA_THRIFT_POM_VERSION}-p4 fi unset IMPALA_THRIFT_JAVA_URL -export IMPALA_THRIFT_PY_VERSION=0.16.0-p3 +export IMPALA_THRIFT_PY_VERSION=0.16.0-p4 unset IMPALA_THRIFT_PY_URL # Find system python versions for testing diff --git a/shell/impala_shell.py b/shell/impala_shell.py index ddf2e2145..5e1c4f918 100755 --- a/shell/impala_shell.py +++ b/shell/impala_shell.py @@ -141,6 +141,10 @@ class ImpalaShell(cmd.Cmd, object): 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 in shell when cancelling a query CANCELLATION_MESSAGE = ' Cancelling Query' # Number of times to attempt cancellation before giving up. @@ -726,7 +730,7 @@ class ImpalaShell(cmd.Cmd, object): return str() # There is no need to reconnect if we are quitting. if not self.imp_client.is_connected() and not self._is_quit_command(args): - print("Connection lost, reconnecting...", file=sys.stderr) + print(ImpalaShell.CONNECTION_LOST_MESSAGE, file=sys.stderr) self._connect() self._validate_database(immediately=True) return args diff --git a/tests/custom_cluster/test_thrift_socket.py b/tests/custom_cluster/test_thrift_socket.py index 4451ccf8d..4461a6820 100644 --- a/tests/custom_cluster/test_thrift_socket.py +++ b/tests/custom_cluster/test_thrift_socket.py @@ -22,6 +22,12 @@ import ssl import sys import time +# 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 IS_REDHAT_DERIVATIVE from tests.common.custom_cluster_test_suite import CustomClusterTestSuite from tests.common.test_vector import ImpalaTestVector @@ -51,7 +57,10 @@ SSL_ARGS = ("--ssl_client_ca_certificate=%s/server-cert.pem " "--hostname=localhost " # Required to match hostname in certificate % (CERT_DIR, CERT_DIR, CERT_DIR)) -IDLE_ARGS = " --idle_client_poll_period_s=3 -v=2" +# IMPALA-12114 was an issue that occurred when the number of idle polls exceeded a limit +# and lead to disconnection. This uses a very short poll period to make these tests +# do more polls and verify the fix for this issue. +IDLE_ARGS = " --idle_client_poll_period_s=1 -v=2" class TestThriftSocket(CustomClusterTestSuite): @@ -74,7 +83,8 @@ class TestThriftSocket(CustomClusterTestSuite): for protocol_dim in create_client_protocol_dimension(): for vector in [ImpalaTestVector([protocol_dim])]: shell_args = ["-Q", "idle_session_timeout=1800"] - self._run_idle_shell(vector, shell_args, 6) + # This uses a longer idle time to verify IMPALA-12114, see comment above. + self._run_idle_shell(vector, shell_args, 12) self.assert_impalad_log_contains('INFO', r'Socket read or peek timeout encountered.*THRIFT_EAGAIN \(timed out\)', expected_count=-1) @@ -89,7 +99,8 @@ class TestThriftSocket(CustomClusterTestSuite): for protocol_dim in create_client_protocol_dimension(): for vector in [ImpalaTestVector([protocol_dim])]: shell_args = ["-Q", "idle_session_timeout=1800", "--ssl"] - self._run_idle_shell(vector, shell_args, 6) + # This uses a longer idle time to verify IMPALA-12114, see comment above. + self._run_idle_shell(vector, shell_args, 12) self.assert_impalad_log_contains('INFO', r'Socket read or peek timeout encountered.*THRIFT_POLL \(timed out\)', expected_count=-1) @@ -97,9 +108,17 @@ class TestThriftSocket(CustomClusterTestSuite): def _run_idle_shell(self, vector, args, idle_time): p = ImpalaShell(vector, args) p.send_cmd("USE functional") - p.send_cmd("SHOW TABLES") + # Running different statements before and after idle allows for more + # precise asserts that can distinguish output from before vs after. + p.send_cmd("SHOW DATABASES") time.sleep(idle_time) p.send_cmd("SHOW TABLES") result = p.get_result() + # Output from before the idle period + assert "functional_parquet" in result.stdout, result.stdout + # Output from after the idle period assert "alltypesaggmultifilesnopart" in result.stdout, result.stdout + # Impala shell reconnects automatically, so we need to be sure that + # it didn't lose the connection. + assert ImpalaShellClass.CONNECTION_LOST_MESSAGE not in result.stderr, result.stderr
