From 13e82d6439bd92f3e025eee7acd635a17d098a36 Mon Sep 17 00:00:00 2001 From: Mikhail Artemenko Date: Tue, 17 Sep 2024 17:45:04 +0000 Subject: [PATCH] fix double visit of uncommitted changes --- src/Coordination/KeeperStorage.cpp | 6 ++- src/Coordination/tests/gtest_coordination.cpp | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index 63dc39092cf..a3ba7402296 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -1777,7 +1777,8 @@ private: auto child_path = (root_fs_path / child_name).generic_string(); const auto actual_child_node_ptr = storage.uncommitted_state.getActualNodeView(child_path, child_node); - if (actual_child_node_ptr == nullptr) /// node was deleted in previous step of multi transaction + /// if node was changed in previous step of multi transaction - skip until the uncommitted state visit + if (actual_child_node_ptr != &child_node) continue; if (checkLimits(actual_child_node_ptr)) @@ -1811,7 +1812,8 @@ private: const auto actual_child_node_ptr = storage.uncommitted_state.getActualNodeView(child_path, child_node); - if (actual_child_node_ptr == nullptr) /// node was deleted in previous step of multi transaction + /// if node was changed in previous step of multi transaction - skip until the uncommitted state visit + if (actual_child_node_ptr != &child_node) continue; if (checkLimits(actual_child_node_ptr)) diff --git a/src/Coordination/tests/gtest_coordination.cpp b/src/Coordination/tests/gtest_coordination.cpp index 46f36fe0039..15d5f460e94 100644 --- a/src/Coordination/tests/gtest_coordination.cpp +++ b/src/Coordination/tests/gtest_coordination.cpp @@ -3738,6 +3738,46 @@ TYPED_TEST(CoordinationTest, TestRemoveRecursiveInMultiRequest) ASSERT_FALSE(exists("/A/B")); ASSERT_FALSE(exists("/A/B/D")); } + + { + SCOPED_TRACE("Recursive Remove For Subtree With Updated Node"); + int create_zxid = ++zxid; + auto ops = prepare_create_tree(); + + /// First create nodes + const auto create_request = std::make_shared(ops, ACLs{}); + storage.preprocessRequest(create_request, 1, 0, create_zxid); + auto create_responses = storage.processRequest(create_request, 1, create_zxid); + ASSERT_EQ(create_responses.size(), 1); + ASSERT_TRUE(is_multi_ok(create_responses[0].response)); + + /// Small limit + int remove_zxid = ++zxid; + ops = { + zkutil::makeSetRequest("/A/B", "", -1), + zkutil::makeRemoveRecursiveRequest("/A", 3), + }; + auto remove_request = std::make_shared(ops, ACLs{}); + storage.preprocessRequest(remove_request, 1, 0, remove_zxid); + auto remove_responses = storage.processRequest(remove_request, 1, remove_zxid); + + ASSERT_EQ(remove_responses.size(), 1); + ASSERT_FALSE(is_multi_ok(remove_responses[0].response)); + + /// Big limit + remove_zxid = ++zxid; + ops[1] = zkutil::makeRemoveRecursiveRequest("/A", 4); + remove_request = std::make_shared(ops, ACLs{}); + storage.preprocessRequest(remove_request, 1, 0, remove_zxid); + remove_responses = storage.processRequest(remove_request, 1, remove_zxid); + + ASSERT_EQ(remove_responses.size(), 1); + ASSERT_TRUE(is_multi_ok(remove_responses[0].response)); + ASSERT_FALSE(exists("/A")); + ASSERT_FALSE(exists("/A/C")); + ASSERT_FALSE(exists("/A/B")); + ASSERT_FALSE(exists("/A/B/D")); + } } TYPED_TEST(CoordinationTest, TestRemoveRecursiveWatches)