From 9077550bb1b0b732842624d35f37b9d433a90582 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 27 Nov 2023 14:25:45 +0000 Subject: [PATCH] Better Readonly metric --- .../ReplicatedMergeTreeAttachThread.cpp | 17 ++++++++++++ .../ReplicatedMergeTreeRestartingThread.cpp | 27 ++++++++++--------- src/Storages/StorageReplicatedMergeTree.cpp | 11 ++------ src/Storages/StorageReplicatedMergeTree.h | 5 +--- 4 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp b/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp index 6b575b7a51c..9dc0a5d04f2 100644 --- a/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp +++ b/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp @@ -2,6 +2,11 @@ #include #include +namespace CurrentMetrics +{ + extern const Metric ReadonlyReplica; +} + namespace DB { @@ -67,6 +72,12 @@ void ReplicatedMergeTreeAttachThread::run() LOG_ERROR(log, "Initialization failed, table will remain readonly. Error: {}", getCurrentExceptionMessage(/* with_stacktrace */ true)); storage.initialization_done = true; } + + if (!storage.is_readonly_metric_set) + { + storage.is_readonly_metric_set = true; + CurrentMetrics::add(CurrentMetrics::ReadonlyReplica); + } } if (!first_try_done.exchange(true)) @@ -74,6 +85,12 @@ void ReplicatedMergeTreeAttachThread::run() if (shutdown_called) { + if (storage.is_readonly_metric_set) + { + storage.is_readonly_metric_set = false; + CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica); + } + LOG_WARNING(log, "Shutdown called, cancelling initialization"); return; } diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp b/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp index 79054ef46da..da30c914fac 100644 --- a/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp +++ b/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp @@ -79,8 +79,9 @@ void ReplicatedMergeTreeRestartingThread::run() if (first_time) { - if (storage.is_readonly) + if (storage.is_readonly && !storage.is_readonly_metric_set) { + storage.is_readonly_metric_set = true; /// We failed to start replication, table is still readonly, so we should increment the metric. See also setNotReadonly(). CurrentMetrics::add(CurrentMetrics::ReadonlyReplica); } @@ -360,21 +361,20 @@ void ReplicatedMergeTreeRestartingThread::setReadonly(bool on_shutdown) return; if (became_readonly) - CurrentMetrics::add(CurrentMetrics::ReadonlyReplica); - - /// Replica was already readonly, but we should decrement the metric, because we are detaching/dropping table. - /// if first pass wasn't done we don't have to decrement because it wasn't incremented in the first place - /// the task should be deactivated if it's full shutdown so no race is present - if (!first_time && on_shutdown) { - CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica); - assert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0); + storage.is_readonly_metric_set = true; + CurrentMetrics::add(CurrentMetrics::ReadonlyReplica); + return; } - if (storage.since_metadata_err_incr_readonly_metric) + /// Replica was already readonly, but we should decrement the metric if it was set because we are detaching/dropping table. + /// the task should be deactivated if it's full shutdown so no race is present + chassert(on_shutdown); + if (storage.is_readonly_metric_set) { + storage.is_readonly_metric_set = false; CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica); - assert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0); + chassert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0); } } @@ -384,10 +384,11 @@ void ReplicatedMergeTreeRestartingThread::setNotReadonly() /// is_readonly is true on startup, but ReadonlyReplica metric is not incremented, /// because we don't want to change this metric if replication is started successfully. /// So we should not decrement it when replica stopped being readonly on startup. - if (storage.is_readonly.compare_exchange_strong(old_val, false) && !first_time) + if (storage.is_readonly.compare_exchange_strong(old_val, false) && storage.is_readonly_metric_set) { + storage.is_readonly_metric_set = false; CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica); - assert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0); + chassert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0); } } diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 2c26f28b84a..3132cd00215 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -4991,9 +4991,9 @@ void StorageReplicatedMergeTree::startupImpl(bool from_attach_thread) /// Do not start replication if ZooKeeper is not configured or there is no metadata in zookeeper if (!has_metadata_in_zookeeper.has_value() || !*has_metadata_in_zookeeper) { - if (!since_metadata_err_incr_readonly_metric) + if (!is_readonly_metric_set) { - since_metadata_err_incr_readonly_metric = true; + is_readonly_metric_set = true; CurrentMetrics::add(CurrentMetrics::ReadonlyReplica); } @@ -5001,13 +5001,6 @@ void StorageReplicatedMergeTree::startupImpl(bool from_attach_thread) return; } - if (since_metadata_err_incr_readonly_metric) - { - since_metadata_err_incr_readonly_metric = false; - CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica); - assert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0); - } - try { auto zookeeper = getZooKeeper(); diff --git a/src/Storages/StorageReplicatedMergeTree.h b/src/Storages/StorageReplicatedMergeTree.h index 429f381dfe0..a8ab8eb7013 100644 --- a/src/Storages/StorageReplicatedMergeTree.h +++ b/src/Storages/StorageReplicatedMergeTree.h @@ -428,10 +428,7 @@ private: /// If false - ZooKeeper is available, but there is no table metadata. It's safe to drop table in this case. std::optional has_metadata_in_zookeeper; - /// during server restart or attach table process, set since_metadata_err_incr_readonly_metric = true and increase readonly metric if has_metadata_in_zookeeper = false. - /// during detach or drop table process, decrease readonly metric if since_metadata_err_incr_readonly_metric = true. - /// during restore replica process, set since_metadata_err_incr_readonly_metric = false and decrease readonly metric if since_metadata_err_incr_readonly_metric = true. - bool since_metadata_err_incr_readonly_metric = false; + bool is_readonly_metric_set = false; static const String default_zookeeper_name; const String zookeeper_name;