[impala] 02/03: IMPALA-12366: Use 2GB as the default for thrift_rpc_max_message_size

[mi...@apache.org]

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

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

commit 81844499b51da092567c510202a4b7de81ecd8af
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Tue Aug 22 10:36:12 2023 -0700

    IMPALA-12366: Use 2GB as the default for thrift_rpc_max_message_size
    
    Thrift 0.16 implemented a limit on the max message size. In IMPALA-11669,
    we added the thrift_rpc_max_message_size parameter and set the default
    size to 1GB. Some existing clusters have needed to tune this parameter
    higher because their workloads use message sizes larger than 1GB (e.g.
    for metadata updates).
    
    Historically, Impala has been able to send and receive 2GB messages,
    so this changes the default value for thrift_rpc_max_message_size
    to 2GB (INT_MAX). This can be reduced in future when Impala can guarantee
    that messages work properly when split up into smaller batches.
    
    TestGracefulShutdown::test_shutdown_idle started failing with this
    change, because it is producing a different error message for one
    of the negative tests. ClientRequestState::ExecShutdownRequest()
    appends some extra explanation when it sees a "Network error" KRPC error,
    and the test expects that extra explanation. This modifies
    ClientRequestState::ExecShutdownRequest() to provide the extra explanation
    for the new error ("Timed out") as well.
    
    Testing:
     - Ran GVO
    
    Change-Id: Ib624201b683966a9feefb8fe45985f3d52d869fc
    Reviewed-on: http://gerrit.cloudera.org:8080/20394
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
    Reviewed-by: Riza Suminto <ri...@cloudera.com>
    Reviewed-by: Michael Smith <mi...@cloudera.com>
---
 be/src/rpc/thrift-util.cc              | 10 ++++++----
 be/src/service/client-request-state.cc |  5 ++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/be/src/rpc/thrift-util.cc b/be/src/rpc/thrift-util.cc
index 502b7c22b..ce88b9b9b 100644
--- a/be/src/rpc/thrift-util.cc
+++ b/be/src/rpc/thrift-util.cc
@@ -17,6 +17,8 @@
 
 #include "rpc/thrift-util.h"
 
+#include <limits>
+
 #include <gtest/gtest.h>
 #include <thrift/config.h>
 
@@ -56,10 +58,10 @@
 
 #include "common/names.h"
 
-DEFINE_int32(thrift_rpc_max_message_size, (1024 * 1024 * 1024),
-    "The maximum size of a message that any RPC that the server will accept. "
-    "Default to 1GB. Setting 0 or negative value will use the default defined in the "
-    "Thrift. The upper limit is 2147483647 bytes.");
+DEFINE_int32(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.");
 
 using namespace apache::thrift;
 using namespace apache::thrift::transport;
diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index f6c0fbb2b..0f9764201 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -983,6 +983,7 @@ Status ClientRequestState::ExecShutdownRequest() {
     }
   }
   string krpc_error = "RemoteShutdown() RPC failed: Network error";
+  string krpc_error2 = "RemoteShutdown() RPC failed: Timed out";
   NetworkAddressPB krpc_addr = MakeNetworkAddressPB(ip_address, port, backend_id,
       ExecEnv::GetInstance()->rpc_mgr()->GetUdsAddressUniqueId());
   std::unique_ptr<ControlServiceProxy> proxy;
@@ -1014,7 +1015,9 @@ Status ClientRequestState::ExecShutdownRequest() {
     string err_string = Substitute(
         "Rpc to $0 failed with error '$1'", NetworkAddressPBToString(krpc_addr), msg);
     // Attempt to detect if the the failure is because of not using a KRPC port.
-    if (backend_port_specified && msg.find(krpc_error) != string::npos) {
+    if (backend_port_specified &&
+           (msg.find(krpc_error) != string::npos ||
+            msg.find(krpc_error2) != string::npos)) {
       // Prior to IMPALA-7985 :shutdown() used the backend port.
       err_string.append(" This may be because the port specified is wrong. You may have"
                         " specified the backend (thrift) port which :shutdown() can no"