You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2024/05/17 19:08:42 UTC
(impala) 01/02: IMPALA-13020 (part 1): Change thrift_rpc_max_message_size to int64_t
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 13df8239d82a61afc3196295a7878ca2ffe91873
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Sun Apr 28 12:44:50 2024 -0700
IMPALA-13020 (part 1): Change thrift_rpc_max_message_size to int64_t
Thrift 0.16.0 introduced a max message size to protect
receivers against a malicious message allocating large
amounts of memory. That limit is a 32-bit signed integer,
so the max value is 2GB. Impala introduced the
thrift_rpc_max_message_size startup option to set that
for Impala's thrift servers.
There are times when Impala wants to send a message that
is larger than 2GB. In particular, the catalog-update
topic for the statestore can exceed 2GBs when there is
a lot of metadata loaded using the old v1 catalog. When
there is a 2GB max message size, the statestore can create
and send a >2GB message, but the impalads will reject
it. This can lead to impalads having stale metadata.
This switches to a patched Thrift that uses an int64_t
for the max message size for C++ code. It does not modify
the limit.
The MaxMessageSize error was being swallowed in TAcceptQueueServer.cpp,
so this fixes that location to always print MaxMessageSize
exceptions.
This is only patching the Thrift C++ library. It does not
patch the Thrift Java library. There are a few reasons for
that:
- This specific issue involves C++ to C++ communication and
will be solved by patching the C++ library.
- C++ is easy to patch as it is built via the native-toolchain.
There is no corresponding mechanism for patching our Java
dependencies (though one could be developed).
- Java modifications have implications for other dependencies
like Hive which use Thrift to communicate with HMS.
For the Java code that uses max message size, this converts
the 64-bit value to 32-bit value by capping the value at
Integer.MAX_VALUE.
Testing:
- Added enough tables to produce a >2GB catalog-topic and
restarted an impalad with a higher limit specific. Without
the patch, the catalog-topic update would be rejected by the
impalad. With the patch, it succeeds.
Change-Id: I681b1849cc565dcb25de8c070c18776ce69cbb87
Reviewed-on: http://gerrit.cloudera.org:8080/21367
Reviewed-by: Michael Smith <mi...@cloudera.com>
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Tested-by: Joe McDonnell <jo...@cloudera.com>
---
be/src/common/init.cc | 2 +-
be/src/rpc/TAcceptQueueServer.cpp | 13 ++++++++++---
be/src/rpc/thrift-server-test.cc | 2 +-
be/src/rpc/thrift-util.cc | 4 ++--
be/src/rpc/thrift-util.h | 4 ++--
be/src/util/backend-gflag-util.cc | 2 +-
bin/impala-config.sh | 12 ++++++------
common/thrift/BackendGflags.thrift | 2 +-
.../main/java/org/apache/impala/service/BackendConfig.java | 5 ++++-
9 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/be/src/common/init.cc b/be/src/common/init.cc
index 9ac9d9784..dca56aa2c 100644
--- a/be/src/common/init.cc
+++ b/be/src/common/init.cc
@@ -83,7 +83,7 @@ DECLARE_string(re2_mem_limit);
DECLARE_string(reserved_words_version);
DECLARE_bool(symbolize_stacktrace);
DECLARE_string(debug_actions);
-DECLARE_int32(thrift_rpc_max_message_size);
+DECLARE_int64(thrift_rpc_max_message_size);
DEFINE_int32(memory_maintenance_sleep_time_ms, 10000, "Sleep time in milliseconds "
"between memory maintenance iterations");
diff --git a/be/src/rpc/TAcceptQueueServer.cpp b/be/src/rpc/TAcceptQueueServer.cpp
index 516811b22..ada6fd7ff 100644
--- a/be/src/rpc/TAcceptQueueServer.cpp
+++ b/be/src/rpc/TAcceptQueueServer.cpp
@@ -43,8 +43,6 @@ DEFINE_int32(accepted_cnxn_setup_thread_pool_size, 2,
"post-accept, pre-setup connection queue in each thrift server set up to service "
"Impala internal and external connections.");
-DECLARE_int32(thrift_rpc_max_message_size);
-
namespace apache {
namespace thrift {
namespace server {
@@ -89,9 +87,18 @@ class TAcceptQueueServer::Task : public Runnable {
}
}
} catch (const TTransportException& ttx) {
- if (ttx.getType() != TTransportException::END_OF_FILE) {
+ // IMPALA-13020: Thrift throws an END_OF_FILE exception when it hits the
+ // max message size. That is always interesting to us, so we specifically
+ // detect "MaxMessageSize" and print it along with advice on how to address it.
+ bool hit_max_message_size =
+ std::string(ttx.what()).find("MaxMessageSize") != std::string::npos;
+ if (ttx.getType() != TTransportException::END_OF_FILE || hit_max_message_size) {
string errStr = string("TAcceptQueueServer client died: ") + ttx.what();
GlobalOutput(errStr.c_str());
+ if (hit_max_message_size) {
+ GlobalOutput("MaxMessageSize errors can be addressed by increasing "
+ "thrift_rpc_max_message_size on the receiving nodes.");
+ }
}
} catch (const std::exception& x) {
GlobalOutput.printf(
diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc
index 425155c94..ab579b094 100644
--- a/be/src/rpc/thrift-server-test.cc
+++ b/be/src/rpc/thrift-server-test.cc
@@ -47,7 +47,7 @@ DECLARE_int32(state_store_port);
DECLARE_int32(beeswax_port);
-DECLARE_int32(thrift_rpc_max_message_size);
+DECLARE_int64(thrift_rpc_max_message_size);
static string IMPALA_HOME(getenv("IMPALA_HOME"));
static const string& SERVER_CERT =
diff --git a/be/src/rpc/thrift-util.cc b/be/src/rpc/thrift-util.cc
index 973544cca..5b3ae17cd 100644
--- a/be/src/rpc/thrift-util.cc
+++ b/be/src/rpc/thrift-util.cc
@@ -58,7 +58,7 @@
#include "common/names.h"
-DEFINE_int32(thrift_rpc_max_message_size, std::numeric_limits<int32_t>::max(),
+DEFINE_int64(thrift_rpc_max_message_size, std::numeric_limits<int32_t>::max(),
"The maximum size of a message for any RPC that the server will accept. "
"Default to the upper limit of 2147483647 bytes (~2GB). "
"Setting 0 or negative value will use the default defined in Thrift.");
@@ -280,7 +280,7 @@ bool IsConnResetTException(const TTransportException& e) {
strstr(e.what(), "SSL_read: Connection reset by peer") != nullptr);
}
-int ThriftRpcMaxMessageSize() {
+int64_t ThriftRpcMaxMessageSize() {
return FLAGS_thrift_rpc_max_message_size <= 0 ? ThriftDefaultMaxMessageSize() :
FLAGS_thrift_rpc_max_message_size;
}
diff --git a/be/src/rpc/thrift-util.h b/be/src/rpc/thrift-util.h
index 0945c1278..e3a7a6948 100644
--- a/be/src/rpc/thrift-util.h
+++ b/be/src/rpc/thrift-util.h
@@ -39,12 +39,12 @@ class TNetworkAddress;
class ThriftServer;
/// Default max message size from Thrift library.
-inline int ThriftDefaultMaxMessageSize() {
+inline int64_t ThriftDefaultMaxMessageSize() {
return apache::thrift::TConfiguration::DEFAULT_MAX_MESSAGE_SIZE;
}
/// Return the effective max message size based on 'thrift_rpc_max_message_size' flag.
-int ThriftRpcMaxMessageSize();
+int64_t ThriftRpcMaxMessageSize();
/// Return the default Thrift's TConfiguration based on given backend config flags.
std::shared_ptr<apache::thrift::TConfiguration> DefaultTConfiguration();
diff --git a/be/src/util/backend-gflag-util.cc b/be/src/util/backend-gflag-util.cc
index f1b94562b..55b8b04f4 100644
--- a/be/src/util/backend-gflag-util.cc
+++ b/be/src/util/backend-gflag-util.cc
@@ -105,7 +105,7 @@ DECLARE_bool(enable_sync_to_latest_event_on_ddls);
DECLARE_bool(pull_table_types_and_comments);
DECLARE_bool(enable_reload_events);
DECLARE_string(geospatial_library);
-DECLARE_int32(thrift_rpc_max_message_size);
+DECLARE_int64(thrift_rpc_max_message_size);
DECLARE_string(file_metadata_reload_properties);
DECLARE_string(java_weigher);
DECLARE_int32(iceberg_reload_new_files_threshold);
diff --git a/bin/impala-config.sh b/bin/impala-config.sh
index 4bcb364d3..cdd1d84a2 100755
--- a/bin/impala-config.sh
+++ b/bin/impala-config.sh
@@ -85,13 +85,13 @@ export USE_AVRO_CPP=${USE_AVRO_CPP:=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_AARCH64=25-051b912729
-export IMPALA_TOOLCHAIN_BUILD_ID_X86_64=413-051b912729
+export IMPALA_TOOLCHAIN_BUILD_ID_AARCH64=34-f93e2c9a86
+export IMPALA_TOOLCHAIN_BUILD_ID_X86_64=422-f93e2c9a86
export IMPALA_TOOLCHAIN_REPO=\
${IMPALA_TOOLCHAIN_REPO:-https://github.com/cloudera/native-toolchain.git}
export IMPALA_TOOLCHAIN_BRANCH=${IMPALA_TOOLCHAIN_BRANCH:-master}
export IMPALA_TOOLCHAIN_COMMIT_HASH=\
-${IMPALA_TOOLCHAIN_COMMIT_HASH-051b912729cdf327572fec5006c2df054b857f50}
+${IMPALA_TOOLCHAIN_COMMIT_HASH-f93e2c9a865c80cafd76b872ad04400877766a2f}
# Compare the build ref in build IDs by removing everything up-to-and-including the
# first hyphen.
if [ "${IMPALA_TOOLCHAIN_BUILD_ID_AARCH64#*-}" \
@@ -222,7 +222,7 @@ export IMPALA_SIMBA_JDBC_DRIVER_VERSION=42-2.6.32.1041
# respect this version). If upgrading IMPALA_THRIFT_PY_VERSION, also upgrade the
# thrift version in shell/ext-py, shell/packaging/requirements.txt, and
# infra/python/deps/requirements.txt.
-export IMPALA_THRIFT_CPP_VERSION=0.16.0-p6
+export IMPALA_THRIFT_CPP_VERSION=0.16.0-p7
unset IMPALA_THRIFT_CPP_URL
if $USE_APACHE_HIVE; then
# Apache Hive 3 clients can't run on thrift versions >= 0.14 (IMPALA-11801)
@@ -230,10 +230,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}-p6
+ export IMPALA_THRIFT_JAVA_VERSION=${IMPALA_THRIFT_POM_VERSION}-p7
fi
unset IMPALA_THRIFT_JAVA_URL
-export IMPALA_THRIFT_PY_VERSION=0.16.0-p6
+export IMPALA_THRIFT_PY_VERSION=0.16.0-p7
unset IMPALA_THRIFT_PY_URL
# Find system python versions for testing
diff --git a/common/thrift/BackendGflags.thrift b/common/thrift/BackendGflags.thrift
index e51b6bad4..73ff305db 100644
--- a/common/thrift/BackendGflags.thrift
+++ b/common/thrift/BackendGflags.thrift
@@ -251,7 +251,7 @@ struct TBackendGflags {
110: required bool skip_resource_checking_on_last_executor_group_set
- 111: required i32 thrift_rpc_max_message_size
+ 111: required i64 thrift_rpc_max_message_size
112: required string file_metadata_reload_properties
diff --git a/fe/src/main/java/org/apache/impala/service/BackendConfig.java b/fe/src/main/java/org/apache/impala/service/BackendConfig.java
index a050fe016..d2a1d7504 100644
--- a/fe/src/main/java/org/apache/impala/service/BackendConfig.java
+++ b/fe/src/main/java/org/apache/impala/service/BackendConfig.java
@@ -405,7 +405,10 @@ public class BackendConfig {
}
public int getThriftRpcMaxMessageSize() {
- return backendCfg_.thrift_rpc_max_message_size;
+ // With IMPALA-13020, the C++ max message size is a 64-bit integer,
+ // but the Java max message size is still 32-bit. Cap the Java value
+ // at Integer.MAX_VALUE;
+ return (int) Math.min(backendCfg_.thrift_rpc_max_message_size, Integer.MAX_VALUE);
}
public String getFileMetadataReloadProperties() {