Intermediate changes

4 files changed, 28 insertions, 30 deletions

diff --git a/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp b/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp
index 65cf8bbce7..927df3c50e 100644
--- a/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp
+++ b/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp
@@ -2,6 +2,11 @@
 #include <Storages/StorageReplicatedMergeTree.h>
 #include <Common/ZooKeeper/IKeeper.h>
 
+namespace CurrentMetrics
+{
+    extern const Metric ReadonlyReplica;
+}
+
 namespace DB
 {
 
@@ -67,6 +72,9 @@ void ReplicatedMergeTreeAttachThread::run()
             LOG_ERROR(log, "Initialization failed, table will remain readonly. Error: {}", getCurrentExceptionMessage(/* with_stacktrace */ true));
             storage.initialization_done = true;
         }
+
+        if (!std::exchange(storage.is_readonly_metric_set, true))
+            CurrentMetrics::add(CurrentMetrics::ReadonlyReplica);
     }
 
     if (!first_try_done.exchange(true))
@@ -74,6 +82,12 @@ void ReplicatedMergeTreeAttachThread::run()
 
     if (shutdown_called)
     {
+        if (std::exchange(storage.is_readonly_metric_set, false))
+        {
+            CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica);
+            chassert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0);
+        }
+
         LOG_WARNING(log, "Shutdown called, cancelling initialization");
         return;
     }
diff --git a/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp b/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp
index 79054ef46d..d83e9c3f80 100644
--- a/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp
+++ b/contrib/clickhouse/src/Storages/MergeTree/ReplicatedMergeTreeRestartingThread.cpp
@@ -79,11 +79,10 @@ void ReplicatedMergeTreeRestartingThread::run()
 
     if (first_time)
     {
-        if (storage.is_readonly)
-        {
+        if (storage.is_readonly && !std::exchange(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);
-        }
+
         /// It does not matter if replication is actually started or not, just notify after the first attempt.
         storage.startup_event.set();
         first_time = false;
@@ -360,21 +359,19 @@ 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);
+        chassert(!storage.is_readonly_metric_set);
+        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
+    if (on_shutdown && std::exchange(storage.is_readonly_metric_set, false))
     {
         CurrentMetrics::sub(CurrentMetrics::ReadonlyReplica);
-        assert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0);
+        chassert(CurrentMetrics::get(CurrentMetrics::ReadonlyReplica) >= 0);
     }
 }
 
@@ -384,10 +381,10 @@ 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) && std::exchange(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/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.cpp b/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.cpp
index 2c272e2ecd..ddf9d743aa 100644
--- a/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.cpp
+++ b/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.cpp
@@ -4820,23 +4820,13 @@ 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)
-        {
-            since_metadata_err_incr_readonly_metric = true;
+        if (!std::exchange(is_readonly_metric_set, true))
             CurrentMetrics::add(CurrentMetrics::ReadonlyReplica);
-        }
 
         LOG_TRACE(log, "No connection to ZooKeeper or no metadata in ZooKeeper, will not startup");
         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/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.h b/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.h
index 794991d8e0..33acf5af83 100644
--- a/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.h
+++ b/contrib/clickhouse/src/Storages/StorageReplicatedMergeTree.h
@@ -427,10 +427,7 @@ private:
     /// If false - ZooKeeper is available, but there is no table metadata. It's safe to drop table in this case.
     std::optional<bool> 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;