From 72a0797b88e9183c7a0f333cb20967b9ad0224f6 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Fri, 29 Dec 2023 15:21:46 +0100 Subject: [PATCH 1/3] keep exception format string in retries ctl --- src/Backups/BackupCoordinationStageSync.cpp | 12 +++---- .../MergeTree/ReplicatedMergeTreeSink.cpp | 6 ++-- src/Storages/MergeTree/ZooKeeperRetries.h | 35 ++----------------- 3 files changed, 12 insertions(+), 41 deletions(-) diff --git a/src/Backups/BackupCoordinationStageSync.cpp b/src/Backups/BackupCoordinationStageSync.cpp index cedcecfd35c..081c2325869 100644 --- a/src/Backups/BackupCoordinationStageSync.cpp +++ b/src/Backups/BackupCoordinationStageSync.cpp @@ -154,14 +154,14 @@ BackupCoordinationStageSync::State BackupCoordinationStageSync::readCurrentState /// If the "alive" node doesn't exist then we don't have connection to the corresponding host. /// This node is ephemeral so probably it will be recreated soon. We use zookeeper retries to wait. /// In worst case when we won't manage to see the alive node for a long time we will just abort the backup. - String message; + const auto suffix = retries_ctl.isLastRetry() ? "" : ", will retry"; if (started) - message = fmt::format("Lost connection to host {}", host); + retries_ctl.setUserError(Exception(ErrorCodes::FAILED_TO_SYNC_BACKUP_OR_RESTORE, + "Lost connection to host {}{}", host, suffix)); else - message = fmt::format("No connection to host {} yet", host); - if (!retries_ctl.isLastRetry()) - message += ", will retry"; - retries_ctl.setUserError(ErrorCodes::FAILED_TO_SYNC_BACKUP_OR_RESTORE, message); + retries_ctl.setUserError(Exception(ErrorCodes::FAILED_TO_SYNC_BACKUP_OR_RESTORE, + "No connection to host {} yet{}", host, suffix)); + state.disconnected_host = host; return state; } diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp b/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp index 8b22c61e012..218ed3bff12 100644 --- a/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp +++ b/src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp @@ -776,7 +776,7 @@ std::pair, bool> ReplicatedMergeTreeSinkImpl:: if (!writing_existing_part) { retries_ctl.setUserError( - ErrorCodes::TABLE_IS_READ_ONLY, "Table is in readonly mode: replica_path={}", storage.replica_path); + Exception(ErrorCodes::TABLE_IS_READ_ONLY, "Table is in readonly mode: replica_path={}", storage.replica_path)); return CommitRetryContext::LOCK_AND_COMMIT; } } @@ -1075,10 +1075,10 @@ std::pair, bool> ReplicatedMergeTreeSinkImpl:: new_retry_controller.actionAfterLastFailedRetry([&] { /// We do not know whether or not data has been inserted in other replicas - new_retry_controller.setUserError( + new_retry_controller.setUserError(Exception( ErrorCodes::UNKNOWN_STATUS_OF_INSERT, "Unknown quorum status. The data was inserted in the local replica but we could not verify quorum. Reason: {}", - new_retry_controller.getLastKeeperErrorMessage()); + new_retry_controller.getLastKeeperErrorMessage())); }); new_retry_controller.retryLoop([&]() diff --git a/src/Storages/MergeTree/ZooKeeperRetries.h b/src/Storages/MergeTree/ZooKeeperRetries.h index 15874b8f675..22282345220 100644 --- a/src/Storages/MergeTree/ZooKeeperRetries.h +++ b/src/Storages/MergeTree/ZooKeeperRetries.h @@ -112,7 +112,7 @@ public: return false; } - void setUserError(std::exception_ptr exception, int code, std::string message) + void setUserError(std::exception_ptr exception, int code, const std::string & message) { if (logger) LOG_TRACE(logger, "ZooKeeperRetriesControl: {}: setUserError: error={} message={}", name, code, message); @@ -127,21 +127,9 @@ public: keeper_error = KeeperError{}; } - template - void setUserError(std::exception_ptr exception, int code, fmt::format_string fmt, Args &&... args) + void setUserError(const Exception & exception) { - setUserError(exception, code, fmt::format(fmt, std::forward(args)...)); - } - - void setUserError(int code, std::string message) - { - setUserError(std::make_exception_ptr(Exception::createDeprecated(message, code)), code, message); - } - - template - void setUserError(int code, fmt::format_string fmt, Args &&... args) - { - setUserError(code, fmt::format(fmt, std::forward(args)...)); + setUserError(std::make_exception_ptr(exception), exception.code(), exception.message()); } void setKeeperError(std::exception_ptr exception, Coordination::Error code, std::string message) @@ -159,23 +147,6 @@ public: user_error = UserError{}; } - template - void setKeeperError(std::exception_ptr exception, Coordination::Error code, fmt::format_string fmt, Args &&... args) - { - setKeeperError(exception, code, fmt::format(fmt, std::forward(args)...)); - } - - void setKeeperError(Coordination::Error code, std::string message) - { - setKeeperError(std::make_exception_ptr(zkutil::KeeperException::createDeprecated(message, code)), code, message); - } - - template - void setKeeperError(Coordination::Error code, fmt::format_string fmt, Args &&... args) - { - setKeeperError(code, fmt::format(fmt, std::forward(args)...)); - } - void stopRetries() { stop_retries = true; } bool isLastRetry() const { return total_failures >= retries_info.max_retries; } From 302ddeb6d0bbf286b216c93c932986f82903ccd9 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Fri, 29 Dec 2023 17:08:13 +0100 Subject: [PATCH 2/3] Update BackupCoordinationStageSync.cpp --- src/Backups/BackupCoordinationStageSync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Backups/BackupCoordinationStageSync.cpp b/src/Backups/BackupCoordinationStageSync.cpp index 081c2325869..c40eb2f1d45 100644 --- a/src/Backups/BackupCoordinationStageSync.cpp +++ b/src/Backups/BackupCoordinationStageSync.cpp @@ -154,7 +154,7 @@ BackupCoordinationStageSync::State BackupCoordinationStageSync::readCurrentState /// If the "alive" node doesn't exist then we don't have connection to the corresponding host. /// This node is ephemeral so probably it will be recreated soon. We use zookeeper retries to wait. /// In worst case when we won't manage to see the alive node for a long time we will just abort the backup. - const auto suffix = retries_ctl.isLastRetry() ? "" : ", will retry"; + const auto * const suffix suffix = retries_ctl.isLastRetry() ? "" : ", will retry"; if (started) retries_ctl.setUserError(Exception(ErrorCodes::FAILED_TO_SYNC_BACKUP_OR_RESTORE, "Lost connection to host {}{}", host, suffix)); From 790ededf80a42ffd1037e2552f84a117f554d733 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Fri, 29 Dec 2023 17:58:24 +0100 Subject: [PATCH 3/3] Update BackupCoordinationStageSync.cpp --- src/Backups/BackupCoordinationStageSync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Backups/BackupCoordinationStageSync.cpp b/src/Backups/BackupCoordinationStageSync.cpp index c40eb2f1d45..2eba3440be9 100644 --- a/src/Backups/BackupCoordinationStageSync.cpp +++ b/src/Backups/BackupCoordinationStageSync.cpp @@ -154,7 +154,7 @@ BackupCoordinationStageSync::State BackupCoordinationStageSync::readCurrentState /// If the "alive" node doesn't exist then we don't have connection to the corresponding host. /// This node is ephemeral so probably it will be recreated soon. We use zookeeper retries to wait. /// In worst case when we won't manage to see the alive node for a long time we will just abort the backup. - const auto * const suffix suffix = retries_ctl.isLastRetry() ? "" : ", will retry"; + const auto * const suffix = retries_ctl.isLastRetry() ? "" : ", will retry"; if (started) retries_ctl.setUserError(Exception(ErrorCodes::FAILED_TO_SYNC_BACKUP_OR_RESTORE, "Lost connection to host {}{}", host, suffix));