From f38dc334d68147a1b5981476930e4221996abace Mon Sep 17 00:00:00 2001 From: Vitaliy Lyudvichenko Date: Wed, 14 Mar 2018 16:19:30 +0300 Subject: [PATCH] Refactor of asyncMulti. [#CLICKHOUSE-2] --- dbms/src/Common/ZooKeeper/ZooKeeper.cpp | 31 ++++---- .../gtest_zkutil_test_multi_exception.cpp | 77 ++++++++++++------- 2 files changed, 64 insertions(+), 44 deletions(-) diff --git a/dbms/src/Common/ZooKeeper/ZooKeeper.cpp b/dbms/src/Common/ZooKeeper/ZooKeeper.cpp index 8d3866a0008..746ed4c609a 100644 --- a/dbms/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/dbms/src/Common/ZooKeeper/ZooKeeper.cpp @@ -978,26 +978,27 @@ ZooKeeper::TryRemoveFuture ZooKeeper::asyncTryRemove(const std::string & path, i ZooKeeper::MultiFuture ZooKeeper::asyncMultiImpl(const zkutil::Ops & ops_, bool throw_exception) { - size_t count = ops_.size(); - auto results_native = std::make_shared>(count); - /// We need to hold all references to ops data until the end of multi callback struct OpsHolder { - std::shared_ptr ops_ptr = std::make_shared(); - std::shared_ptr> ops_raw_ptr = std::make_shared>(); + std::shared_ptr ops_ptr; + std::shared_ptr> ops_native; + std::shared_ptr> op_results_native; } holder; - for (const auto & op : ops_) - { - holder.ops_ptr->emplace_back(op->clone()); - holder.ops_raw_ptr->push_back(*holder.ops_ptr->back()->data); - } + /// Copy ops (swallow copy) + holder.ops_ptr = std::make_shared(ops_); + /// Copy native ops to contiguous vector + holder.ops_native = std::make_shared>(); + for (const OpPtr & op : *holder.ops_ptr) + holder.ops_native->push_back(*op->data); + /// Allocate native result holders + holder.op_results_native = std::make_shared>(holder.ops_ptr->size()); - MultiFuture future{ [throw_exception, results_native, holder, zookeeper=this] (int rc) { + MultiFuture future{ [throw_exception, holder, zookeeper=this] (int rc) { OpResultsAndCode res; res.code = rc; - convertOpResults(*results_native, res.results, zookeeper); + convertOpResults(*holder.op_results_native, res.results, zookeeper); res.ops_ptr = holder.ops_ptr; if (throw_exception && rc != ZOK) throw zkutil::KeeperException(rc); @@ -1015,9 +1016,9 @@ ZooKeeper::MultiFuture ZooKeeper::asyncMultiImpl(const zkutil::Ops & ops_, bool if (expired()) throw KeeperException(ZINVALIDSTATE); - auto & ops = *holder.ops_raw_ptr; - - int32_t code = zoo_amulti(impl, static_cast(ops.size()), ops.data(), results_native->data(), + int32_t code = zoo_amulti(impl, static_cast(holder.ops_native->size()), + holder.ops_native->data(), + holder.op_results_native->data(), [] (int rc, const void * data) { MultiFuture::TaskPtr owned_task = diff --git a/dbms/src/Common/ZooKeeper/tests/gtest_zkutil_test_multi_exception.cpp b/dbms/src/Common/ZooKeeper/tests/gtest_zkutil_test_multi_exception.cpp index 4d1d05b54e1..695b9949a66 100644 --- a/dbms/src/Common/ZooKeeper/tests/gtest_zkutil_test_multi_exception.cpp +++ b/dbms/src/Common/ZooKeeper/tests/gtest_zkutil_test_multi_exception.cpp @@ -6,6 +6,9 @@ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wsign-compare" #include +#include + + #pragma GCC diagnostic pop using namespace DB; @@ -32,32 +35,32 @@ TEST(zkutil, multi_nice_exception_msg) zkutil::Ops ops; ASSERT_NO_THROW( - zookeeper->tryRemoveRecursive("/clickhouse_test_zkutil_multi"); + zookeeper->tryRemoveRecursive("/clickhouse_test/zkutil_multi"); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi", "_", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); zookeeper->multi(ops); ); try { ops.clear(); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/c", "_", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Remove("/clickhouse_test_zkutil_multi/c", -1)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/a", "BadBoy", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/b", "_", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/c", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Remove("/clickhouse_test/zkutil_multi/c", -1)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/a", "BadBoy", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/b", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); zookeeper->multi(ops); FAIL(); } catch (...) { - zookeeper->tryRemoveRecursive("/clickhouse_test_zkutil_multi"); + zookeeper->tryRemoveRecursive("/clickhouse_test/zkutil_multi"); String msg = getCurrentExceptionMessage(false); - bool msg_has_reqired_patterns = msg.find("/clickhouse_test_zkutil_multi/a") != std::string::npos && msg.find("#2") != std::string::npos; + bool msg_has_reqired_patterns = msg.find("/clickhouse_test/zkutil_multi/a") != std::string::npos && msg.find("#2") != std::string::npos; EXPECT_TRUE(msg_has_reqired_patterns) << msg; } } @@ -69,7 +72,7 @@ TEST(zkutil, multi_async) auto acl = zookeeper->getDefaultACL(); zkutil::Ops ops; - zookeeper->tryRemoveRecursive("/clickhouse_test_zkutil_multi"); + zookeeper->tryRemoveRecursive("/clickhouse_test/zkutil_multi"); { ops.clear(); @@ -78,8 +81,8 @@ TEST(zkutil, multi_async) { ops.clear(); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi", "", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/a", "", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi", "", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/a", "", acl, zkutil::CreateMode::Persistent)); auto fut = zookeeper->tryAsyncMulti(ops); ops.clear(); @@ -97,11 +100,11 @@ TEST(zkutil, multi_async) for (size_t i = 0; i < 10000; ++i) { ops.clear(); - ops.emplace_back(new zkutil::Op::Remove("/clickhouse_test_zkutil_multi", -1)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi", "_", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Check("/clickhouse_test_zkutil_multi", -1)); - ops.emplace_back(new zkutil::Op::SetData("/clickhouse_test_zkutil_multi", "xxx", 42)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Remove("/clickhouse_test/zkutil_multi", -1)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Check("/clickhouse_test/zkutil_multi", -1)); + ops.emplace_back(new zkutil::Op::SetData("/clickhouse_test/zkutil_multi", "xxx", 42)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); futures.emplace_back(zookeeper->asyncMulti(ops)); } @@ -115,8 +118,8 @@ TEST(zkutil, multi_async) { ops.clear(); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi", "_", acl, zkutil::CreateMode::Persistent)); - ops.emplace_back(new zkutil::Op::Create("/clickhouse_test_zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi", "_", acl, zkutil::CreateMode::Persistent)); + ops.emplace_back(new zkutil::Op::Create("/clickhouse_test/zkutil_multi/a", "_", acl, zkutil::CreateMode::Persistent)); auto fut = zookeeper->tryAsyncMulti(ops); ops.clear(); @@ -128,11 +131,30 @@ TEST(zkutil, multi_async) } } +/// Run this test under sudo +TEST(zkutil, multi_async_libzookeeper_segfault) +{ + auto zookeeper = std::make_unique("localhost:2181", "", 1000); + zkutil::Ops ops; + + ops.emplace_back(new zkutil::Op::Check("/clickhouse_test/zkutil_multi", 0)); + + /// Uncomment to test + //auto cmd = ShellCommand::execute("sudo service zookeeper restart"); + //cmd->wait(); + + auto future = zookeeper->asyncMulti(ops); + auto res = future.get(); + + EXPECT_TRUE(zkutil::isUnrecoverableErrorCode(res.code)); +} + TEST(zkutil, multi_create_sequential) { try { + /// Create chroot node firstly auto zookeeper = std::make_unique("localhost:2181"); zookeeper->createAncestors("/clickhouse_test/"); @@ -144,17 +166,14 @@ TEST(zkutil, multi_create_sequential) zookeeper->tryRemoveRecursive(base_path); zookeeper->createAncestors(base_path + "/"); - String entry_path = base_path + "/queue-"; - ops.emplace_back(new zkutil::Op::Create(entry_path, "", acl, zkutil::CreateMode::EphemeralSequential)); + String sequential_node_prefix = base_path + "/queue-"; + ops.emplace_back(new zkutil::Op::Create(sequential_node_prefix, "", acl, zkutil::CreateMode::EphemeralSequential)); zkutil::OpResultsPtr results = zookeeper->multi(ops); - zkutil::OpResult & result = results->at(0); + zkutil::OpResult & sequential_node_result_op = results->at(0); - EXPECT_TRUE(result.value != nullptr); - - String path = result.value; - std::cout << path << "\n"; - - EXPECT_TRUE(startsWith(result.value, entry_path)); + EXPECT_FALSE(sequential_node_result_op.value.empty()); + EXPECT_GT(sequential_node_result_op.value.length(), sequential_node_prefix.length()); + EXPECT_EQ(sequential_node_result_op.value.substr(0, sequential_node_prefix.length()), sequential_node_prefix); } catch (...) {