Merge pull request #40158 from ClickHouse/make_tests_less_flaky

Set sync_request_timeout to 10 to avoid reconnections in tests

src/Client/ClientBase.cpp

@@ -1491,7 +1491,7 @@ void ClientBase::processParsedSingleQuery(const String & full_query, const Strin
         if (with_output && with_output->settings_ast)
             apply_query_settings(*with_output->settings_ast);
 
-        if (!connection->checkConnected())
+        if (!connection->checkConnected(connection_parameters.timeouts))
             connect();
 
         ASTPtr input_function;
@@ -1835,7 +1835,7 @@ bool ClientBase::executeMultiQuery(const String & all_queries_text)
 
                     have_error = false;
 
-                    if (!connection->checkConnected())
+                    if (!connection->checkConnected(connection_parameters.timeouts))
                         connect();
                 }
 
@@ -2052,7 +2052,7 @@ void ClientBase::runInteractive()
             /// Client-side exception during query execution can result in the loss of
             /// sync in the connection protocol.
             /// So we reconnect and allow to enter the next query.
-            if (!connection->checkConnected())
+            if (!connection->checkConnected(connection_parameters.timeouts))
                 connect();
         }
     }

src/Client/Connection.cpp

@@ -69,8 +69,7 @@ Connection::Connection(const String & host_, UInt16 port_,
     const String & cluster_secret_,
     const String & client_name_,
     Protocol::Compression compression_,
-    Protocol::Secure secure_,
-    Poco::Timespan sync_request_timeout_)
+    Protocol::Secure secure_)
     : host(host_), port(port_), default_database(default_database_)
     , user(user_), password(password_), quota_key(quota_key_)
     , cluster(cluster_)
@@ -78,7 +77,6 @@ Connection::Connection(const String & host_, UInt16 port_,
     , client_name(client_name_)
     , compression(compression_)
     , secure(secure_)
-    , sync_request_timeout(sync_request_timeout_)
     , log_wrapper(*this)
 {
     /// Don't connect immediately, only on first need.
@@ -386,7 +384,7 @@ void Connection::forceConnected(const ConnectionTimeouts & timeouts)
     {
         connect(timeouts);
     }
-    else if (!ping())
+    else if (!ping(timeouts))
     {
         LOG_TRACE(log_wrapper.get(), "Connection was closed, will reconnect.");
         connect(timeouts);
@@ -406,11 +404,11 @@ void Connection::sendClusterNameAndSalt()
 }
 #endif
 
-bool Connection::ping()
+bool Connection::ping(const ConnectionTimeouts & timeouts)
 {
     try
     {
-        TimeoutSetter timeout_setter(*socket, sync_request_timeout, true);
+        TimeoutSetter timeout_setter(*socket, timeouts.sync_request_timeout, true);
 
         UInt64 pong = 0;
         writeVarUInt(Protocol::Client::Ping, *out);
@@ -454,7 +452,7 @@ TablesStatusResponse Connection::getTablesStatus(const ConnectionTimeouts & time
     if (!connected)
         connect(timeouts);
 
-    TimeoutSetter timeout_setter(*socket, sync_request_timeout, true);
+    TimeoutSetter timeout_setter(*socket, timeouts.sync_request_timeout, true);
 
     writeVarUInt(Protocol::Client::TablesStatusRequest, *out);
     request.write(*out, server_revision);

src/Client/Connection.h

@@ -56,8 +56,7 @@ public:
         const String & cluster_secret_,
         const String & client_name_,
         Protocol::Compression compression_,
-        Protocol::Secure secure_,
-        Poco::Timespan sync_request_timeout_ = Poco::Timespan(DBMS_DEFAULT_SYNC_REQUEST_TIMEOUT_SEC, 0));
+        Protocol::Secure secure_);
 
     ~Connection() override;
 
@@ -125,7 +124,7 @@ public:
 
     bool isConnected() const override { return connected; }
 
-    bool checkConnected() override { return connected && ping(); }
+    bool checkConnected(const ConnectionTimeouts & timeouts) override { return connected && ping(timeouts); }
 
     void disconnect() override;
 
@@ -208,8 +207,6 @@ private:
       */
     ThrottlerPtr throttler;
 
-    Poco::Timespan sync_request_timeout;
-
     /// From where to read query execution result.
     std::shared_ptr<ReadBuffer> maybe_compressed_in;
     std::unique_ptr<NativeReader> block_in;
@@ -254,7 +251,7 @@ private:
 #if USE_SSL
     void sendClusterNameAndSalt();
 #endif
-    bool ping();
+    bool ping(const ConnectionTimeouts & timeouts);
 
     Block receiveData();
     Block receiveLogData();

src/Client/ConnectionParameters.cpp

@@ -69,6 +69,8 @@ ConnectionParameters::ConnectionParameters(const Poco::Util::AbstractConfigurati
             Poco::Timespan(config.getInt("send_timeout", DBMS_DEFAULT_SEND_TIMEOUT_SEC), 0),
             Poco::Timespan(config.getInt("receive_timeout", DBMS_DEFAULT_RECEIVE_TIMEOUT_SEC), 0),
             Poco::Timespan(config.getInt("tcp_keep_alive_timeout", 0), 0));
+
+    timeouts.sync_request_timeout = Poco::Timespan(config.getInt("sync_request_timeout", DBMS_DEFAULT_SYNC_REQUEST_TIMEOUT_SEC), 0);
 }
 
 ConnectionParameters::ConnectionParameters(const Poco::Util::AbstractConfiguration & config)

src/Client/IServerConnection.h

@@ -122,7 +122,7 @@ public:
     virtual bool isConnected() const = 0;
 
     /// Check if connection is still active with ping request.
-    virtual bool checkConnected() = 0;
+    virtual bool checkConnected(const ConnectionTimeouts & /*timeouts*/) = 0;
 
     /** Disconnect.
       * This may be used, if connection is left in unsynchronised state

src/Client/LocalConnection.h

@@ -122,7 +122,7 @@ public:
 
     bool isConnected() const override { return true; }
 
-    bool checkConnected() override { return true; }
+    bool checkConnected(const ConnectionTimeouts & /*timeouts*/) override { return true; }
 
     void disconnect() override {}
 

src/IO/ConnectionTimeouts.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <Core/Defines.h>
 #include <Interpreters/Context_fwd.h>
 
 #include <Poco/Timespan.h>
@@ -22,6 +23,9 @@ struct ConnectionTimeouts
     Poco::Timespan hedged_connection_timeout;
     Poco::Timespan receive_data_timeout;
 
+    /// Timeout for synchronous request-result protocol call (like Ping or TablesStatus)
+    Poco::Timespan sync_request_timeout = Poco::Timespan(DBMS_DEFAULT_SYNC_REQUEST_TIMEOUT_SEC, 0);
+
     ConnectionTimeouts() = default;
 
     ConnectionTimeouts(Poco::Timespan connection_timeout_,

tests/config/client_config.xml

@@ -11,4 +11,7 @@
             </invalidCertificateHandler>
         </client>
     </openSSL>
+
+    <!-- Default timeout is 5 sec. Set it to 10 to avoid tests flakiness with slow builds (debug, tsan) -->
+    <sync_request_timeout>10</sync_request_timeout>
 </config>