Merge pull request ClickHouse#86674 from ClickHouse/backport/25.8/86554

Backport ClickHouse#86554 to 25.8: Check feature flag when adding RemoveRecursive request to Multi

Author: antonio2368

src/Common/ZooKeeper/Types.h

@@ -30,12 +30,14 @@ using AsyncResponses = std::vector<std::pair<std::string, std::future<R>>>;
 
 Coordination::RequestPtr makeCreateRequest(const std::string & path, const std::string & data, int create_mode, bool ignore_if_exists = false);
 Coordination::RequestPtr makeRemoveRequest(const std::string & path, int version);
-Coordination::RequestPtr makeRemoveRecursiveRequest(const std::string & path, uint32_t remove_nodes_limit);
 Coordination::RequestPtr makeSetRequest(const std::string & path, const std::string & data, int version);
 Coordination::RequestPtr makeCheckRequest(const std::string & path, int version);
 Coordination::RequestPtr makeGetRequest(const std::string & path, Coordination::WatchCallbackPtr watch = nullptr);
 Coordination::RequestPtr makeListRequest(const std::string & path, Coordination::ListRequestType list_request_type = Coordination::ListRequestType::ALL, Coordination::WatchCallbackPtr watch = nullptr);
 Coordination::RequestPtr makeSimpleListRequest(const std::string & path, Coordination::WatchCallbackPtr watch = nullptr);
 Coordination::RequestPtr makeExistsRequest(const std::string & path, Coordination::WatchCallbackPtr watch = nullptr);
 
+template <class Client>
+Coordination::RequestPtr makeRemoveRecursiveRequest(const Client & client, const std::string & path, uint32_t remove_nodes_limit);
+
 }

src/Common/ZooKeeper/ZooKeeper.cpp

@@ -1727,14 +1727,21 @@ Coordination::RequestPtr makeRemoveRequest(const std::string & path, int version
     return request;
 }
 
-Coordination::RequestPtr makeRemoveRecursiveRequest(const std::string & path, uint32_t remove_nodes_limit)
+template <class Client>
+Coordination::RequestPtr makeRemoveRecursiveRequest(const Client & client, const std::string & path, uint32_t remove_nodes_limit)
 {
+    if (!client.isFeatureEnabled(DB::KeeperFeatureFlag::REMOVE_RECURSIVE))
+        throw DB::Exception(DB::ErrorCodes::LOGICAL_ERROR, "Trying to use RemoveRecursive request while Keeper doesn't support it or feature flag is disabled");
+
     auto request = std::make_shared<Coordination::ZooKeeperRemoveRecursiveRequest>();
     request->path = path;
     request->remove_nodes_limit = remove_nodes_limit;
     return request;
 }
 
+template Coordination::RequestPtr makeRemoveRecursiveRequest<zkutil::ZooKeeper>(const zkutil::ZooKeeper & client, const std::string & path, uint32_t remove_nodes_limit);
+template Coordination::RequestPtr makeRemoveRecursiveRequest<DB::ZooKeeperWithFaultInjection>(const DB::ZooKeeperWithFaultInjection & client, const std::string & path, uint32_t remove_nodes_limit);
+
 Coordination::RequestPtr makeSetRequest(const std::string & path, const std::string & data, int version)
 {
     auto request = std::make_shared<Coordination::ZooKeeperSetRequest>();

src/Coordination/tests/gtest_coordination_storage.cpp

@@ -367,6 +367,17 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveRequest)
     }
 }
 
+namespace
+{
+Coordination::RequestPtr makeRemoveRecursiveRequest(const std::string & path, uint32_t remove_nodes_limit)
+{
+    auto request = std::make_shared<Coordination::ZooKeeperRemoveRecursiveRequest>();
+    request->path = path;
+    request->remove_nodes_limit = remove_nodes_limit;
+    return request;
+}
+}
+
 TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
 {
     using namespace DB;
@@ -436,7 +447,7 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
         int new_zxid = ++zxid;
         auto ops = prepare_create_tree();
 
-        ops.push_back(zkutil::makeRemoveRecursiveRequest("/A", 4));
+        ops.push_back(makeRemoveRecursiveRequest("/A", 4));
         const auto request = std::make_shared<ZooKeeperMultiRequest>(ops, ACLs{});
 
         storage.preprocessRequest(request, 1, 0, new_zxid);
@@ -457,7 +468,7 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
         auto ops = prepare_create_tree();
 
         ops.push_back(zkutil::makeRemoveRequest("/A/C", -1));
-        ops.push_back(zkutil::makeRemoveRecursiveRequest("/A", 3));
+        ops.push_back(makeRemoveRecursiveRequest("/A", 3));
         const auto request = std::make_shared<ZooKeeperMultiRequest>(ops, ACLs{});
 
         storage.preprocessRequest(request, 1, 0, new_zxid);
@@ -497,7 +508,7 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
         int remove_zxid = ++zxid;
         ops = {
             zkutil::makeRemoveRequest("/A/C", -1),
-            zkutil::makeRemoveRecursiveRequest("/A", 3),
+            makeRemoveRecursiveRequest("/A", 3),
         };
         const auto remove_request = std::make_shared<ZooKeeperMultiRequest>(ops, ACLs{});
 
@@ -528,7 +539,7 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
         int remove_zxid = ++zxid;
         ops = {
             zkutil::makeSetRequest("/A/B", "", -1),
-            zkutil::makeRemoveRecursiveRequest("/A", 3),
+            makeRemoveRecursiveRequest("/A", 3),
         };
         auto remove_request = std::make_shared<ZooKeeperMultiRequest>(ops, ACLs{});
         storage.preprocessRequest(remove_request, 1, 0, remove_zxid);
@@ -539,7 +550,7 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
 
         /// Big limit
         remove_zxid = ++zxid;
-        ops[1] = zkutil::makeRemoveRecursiveRequest("/A", 4);
+        ops[1] = makeRemoveRecursiveRequest("/A", 4);
         remove_request = std::make_shared<ZooKeeperMultiRequest>(ops, ACLs{});
         storage.preprocessRequest(remove_request, 1, 0, remove_zxid);
         remove_responses = storage.processRequest(remove_request, 1, remove_zxid);
@@ -562,7 +573,7 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest)
             zkutil::makeCreateRequest("/A", "", zkutil::CreateMode::Persistent),
             zkutil::makeCreateRequest("/A/B", "", zkutil::CreateMode::Persistent),
             zkutil::makeCreateRequest("/A/CCCCCCCCCCCC", "", zkutil::CreateMode::Persistent),
-            zkutil::makeRemoveRecursiveRequest("/A", 3),
+            makeRemoveRecursiveRequest("/A", 3),
         };
         auto remove_request = std::make_shared<ZooKeeperMultiRequest>(ops, ACLs{});
         storage.preprocessRequest(remove_request, 1, 0, new_zxid);

src/Storages/ObjectStorageQueue/ObjectStorageQueueMetadata.cpp

@@ -771,7 +771,7 @@ void ObjectStorageQueueMetadata::unregisterNonActive(const StorageID & storage_i
                 if (supports_remove_recursive)
                 {
                     requests.push_back(zkutil::makeCheckRequest(registry_path, stat.version));
-                    requests.push_back(zkutil::makeRemoveRecursiveRequest(zookeeper_path, std::numeric_limits<uint32_t>::max()));
+                    requests.push_back(zkutil::makeRemoveRecursiveRequest(*zk_client, zookeeper_path, std::numeric_limits<uint32_t>::max()));
                 }
                 else
                 {