From 82c06920e35238e6d411396e1d67d9fc603d5a04 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 29 Jul 2024 12:27:12 +0000 Subject: [PATCH 1/3] Try to fix: ALL_CONNECTION_TRIES_FAILED with parallel replicas --- src/Client/ConnectionEstablisher.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Client/ConnectionEstablisher.cpp b/src/Client/ConnectionEstablisher.cpp index 4bab643ed31..8cebe7a6183 100644 --- a/src/Client/ConnectionEstablisher.cpp +++ b/src/Client/ConnectionEstablisher.cpp @@ -38,7 +38,7 @@ void ConnectionEstablisher::run(ConnectionEstablisher::TryResult & result, std:: try { ProfileEvents::increment(ProfileEvents::DistributedConnectionTries); - result.entry = pool->get(*timeouts, settings, /* force_connected = */ false); + result.entry = pool->get(*timeouts, settings, /* force_connected = */ true); AsyncCallbackSetter async_setter(&*result.entry, std::move(async_callback)); UInt64 server_revision = 0; From 2cae0cb5ecedc2fd041def829b35bdf4dbb50f2f Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 29 Jul 2024 20:29:15 +0000 Subject: [PATCH 2/3] force_connected flag for connection establisher --- src/Client/ConnectionEstablisher.cpp | 4 ++-- src/Client/ConnectionEstablisher.h | 4 +++- src/QueryPipeline/RemoteQueryExecutor.cpp | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Client/ConnectionEstablisher.cpp b/src/Client/ConnectionEstablisher.cpp index 8cebe7a6183..f96546846c7 100644 --- a/src/Client/ConnectionEstablisher.cpp +++ b/src/Client/ConnectionEstablisher.cpp @@ -33,12 +33,12 @@ ConnectionEstablisher::ConnectionEstablisher( { } -void ConnectionEstablisher::run(ConnectionEstablisher::TryResult & result, std::string & fail_message) +void ConnectionEstablisher::run(ConnectionEstablisher::TryResult & result, std::string & fail_message, bool force_connected) { try { ProfileEvents::increment(ProfileEvents::DistributedConnectionTries); - result.entry = pool->get(*timeouts, settings, /* force_connected = */ true); + result.entry = pool->get(*timeouts, settings, force_connected); AsyncCallbackSetter async_setter(&*result.entry, std::move(async_callback)); UInt64 server_revision = 0; diff --git a/src/Client/ConnectionEstablisher.h b/src/Client/ConnectionEstablisher.h index a3a01e63246..304ec4d34b4 100644 --- a/src/Client/ConnectionEstablisher.h +++ b/src/Client/ConnectionEstablisher.h @@ -24,7 +24,9 @@ public: const QualifiedTableName * table_to_check = nullptr); /// Establish connection and save it in result, write possible exception message in fail_message. - void run(TryResult & result, std::string & fail_message); + /// The connection is returned from the pool, it can be stale. Use force_connected flag + /// to ensure that connection is working one + void run(TryResult & result, std::string & fail_message, bool force_connected = false); /// Set async callback that will be called when reading from socket blocks. void setAsyncCallback(AsyncCallback async_callback_) { async_callback = std::move(async_callback_); } diff --git a/src/QueryPipeline/RemoteQueryExecutor.cpp b/src/QueryPipeline/RemoteQueryExecutor.cpp index b08f2002f64..09ea6a9fb3c 100644 --- a/src/QueryPipeline/RemoteQueryExecutor.cpp +++ b/src/QueryPipeline/RemoteQueryExecutor.cpp @@ -89,12 +89,12 @@ RemoteQueryExecutor::RemoteQueryExecutor( auto table_name = main_table.getQualifiedName(); ConnectionEstablisher connection_establisher(pool, &timeouts, current_settings, log, &table_name); - connection_establisher.run(result, fail_message); + connection_establisher.run(result, fail_message, /*force_connected=*/ true); } else { ConnectionEstablisher connection_establisher(pool, &timeouts, current_settings, log, nullptr); - connection_establisher.run(result, fail_message); + connection_establisher.run(result, fail_message, /*force_connected=*/ true); } std::vector connection_entries; From a70cdb8bba5503f3723a2e29957617ea06106c4d Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Tue, 30 Jul 2024 15:37:59 +0000 Subject: [PATCH 3/3] Add comment regarding default value for force_connected --- src/Client/ConnectionEstablisher.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Client/ConnectionEstablisher.h b/src/Client/ConnectionEstablisher.h index 304ec4d34b4..ff071e59aea 100644 --- a/src/Client/ConnectionEstablisher.h +++ b/src/Client/ConnectionEstablisher.h @@ -24,8 +24,12 @@ public: const QualifiedTableName * table_to_check = nullptr); /// Establish connection and save it in result, write possible exception message in fail_message. - /// The connection is returned from the pool, it can be stale. Use force_connected flag - /// to ensure that connection is working one + /// The connection is returned from connection pool and it can be stale. Use force_connected flag to ensure that connection is working one. + /// NOTE: force_connected is false by default due to the following consideration ... + /// When true, it implies sending a Ping packet to another peer and, if it fails - reestablishing the connection. + /// Ping-Pong round trip can be unnecessary in case of connection is still alive. + /// So, the optimistic approach is used by default. In this case, stale connections can be handled by retrying, + /// - see ConnectionPoolWithFailover, as example void run(TryResult & result, std::string & fail_message, bool force_connected = false); /// Set async callback that will be called when reading from socket blocks.