From 94d6d8366ca55ab4907487e7271f3f70a8970998 Mon Sep 17 00:00:00 2001 From: zilai Date: Sat, 7 Oct 2023 01:55:19 -0700 Subject: [PATCH] fix review comments and add uts for multi-shards on same pg --- src/lib/homestore_backend/hs_homeobject.cpp | 2 +- src/lib/homestore_backend/hs_homeobject.hpp | 2 +- .../homestore_backend/hs_shard_manager.cpp | 8 +-- .../replication_state_machine.cpp | 2 +- .../tests/test_shard_manager.cpp | 56 +++++++++++++++++++ 5 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/lib/homestore_backend/hs_homeobject.cpp b/src/lib/homestore_backend/hs_homeobject.cpp index b8d00493..7a15b7ab 100644 --- a/src/lib/homestore_backend/hs_homeobject.cpp +++ b/src/lib/homestore_backend/hs_homeobject.cpp @@ -52,7 +52,7 @@ void HSHomeObject::init_homestore() { {HS_SERVICE::LOG_REPLICATED, hs_format_params{.size_pct = 10.0}}, {HS_SERVICE::LOG_LOCAL, hs_format_params{.size_pct = 0.1}}, // TODO: Remove this after HS disables LOG_LOCAL {HS_SERVICE::REPLICATION, - hs_format_params{.size_pct = 80.0, + hs_format_params{.size_pct = 79.0, .num_chunks = 65000, .block_size = 1024, .alloc_type = blk_allocator_type_t::append, diff --git a/src/lib/homestore_backend/hs_homeobject.hpp b/src/lib/homestore_backend/hs_homeobject.hpp index ca16144f..7080c387 100644 --- a/src/lib/homestore_backend/hs_homeobject.hpp +++ b/src/lib/homestore_backend/hs_homeobject.hpp @@ -114,7 +114,7 @@ class HSHomeObject : public HomeObjectImpl { void on_shard_message_commit(int64_t lsn, sisl::blob const& header, homestore::MultiBlkId const& blkids, homestore::ReplDev* repl_dev, cintrusive< homestore::repl_req_ctx >& hs_ctx); - ShardManager::Result< homestore::chunk_num_t > get_shard_chunk(shard_id_t id) const; + std::optional< homestore::chunk_num_t > get_shard_chunk(shard_id_t id) const; std::optional< homestore::chunk_num_t > get_any_chunk_id(pg_id_t const pg); diff --git a/src/lib/homestore_backend/hs_shard_manager.cpp b/src/lib/homestore_backend/hs_shard_manager.cpp index a55a1d28..a1f21bcd 100644 --- a/src/lib/homestore_backend/hs_shard_manager.cpp +++ b/src/lib/homestore_backend/hs_shard_manager.cpp @@ -188,12 +188,12 @@ void HSHomeObject::update_shard_in_map(const ShardInfo& shard_info) { hs_shard->update_info(shard_info); } -ShardManager::Result< homestore::chunk_num_t > HSHomeObject::get_shard_chunk(shard_id_t id) const { +std::optional< homestore::chunk_num_t > HSHomeObject::get_shard_chunk(shard_id_t id) const { std::scoped_lock lock_guard(_shard_lock); auto shard_iter = _shard_map.find(id); - if (shard_iter == _shard_map.end()) { return folly::makeUnexpected(ShardError::UNKNOWN_SHARD); } + if (shard_iter == _shard_map.end()) { return std::nullopt; } auto hs_shard = d_cast< HS_Shard* >((*shard_iter->second).get()); - return hs_shard->sb_->chunk_id; + return std::make_optional< homestore::chunk_num_t >(hs_shard->sb_->chunk_id); } std::optional< homestore::chunk_num_t > HSHomeObject::get_any_chunk_id(pg_id_t const pg_id) { @@ -207,7 +207,7 @@ std::optional< homestore::chunk_num_t > HSHomeObject::get_any_chunk_id(pg_id_t c } auto& shards = pg->shards_; - if (shards.empty()) { return std::optional< homestore::chunk_num_t >(); } + if (shards.empty()) { return std::nullopt; } auto hs_shard = d_cast< HS_Shard* >(shards.front().get()); // cache it; pg->any_allocated_chunk_id_ = hs_shard->sb_->chunk_id; diff --git a/src/lib/homestore_backend/replication_state_machine.cpp b/src/lib/homestore_backend/replication_state_machine.cpp index d0027a5d..9b3231c5 100644 --- a/src/lib/homestore_backend/replication_state_machine.cpp +++ b/src/lib/homestore_backend/replication_state_machine.cpp @@ -51,7 +51,7 @@ homestore::blk_alloc_hints ReplicationStateMachine::get_blk_alloc_hints(sisl::bl case ReplicationMessageType::SEAL_SHARD_MSG: { auto chunk_id = home_object_->get_shard_chunk(msg_header->shard_id); - RELEASE_ASSERT(!!chunk_id, "unknown shard id to get binded chunk"); + RELEASE_ASSERT(chunk_id.has_value(), "unknown shard id to get binded chunk"); homestore::blk_alloc_hints hints; hints.chunk_id_hint = chunk_id.value(); return hints; diff --git a/src/lib/homestore_backend/tests/test_shard_manager.cpp b/src/lib/homestore_backend/tests/test_shard_manager.cpp index 9e4e8155..4ae77f7a 100644 --- a/src/lib/homestore_backend/tests/test_shard_manager.cpp +++ b/src/lib/homestore_backend/tests/test_shard_manager.cpp @@ -120,6 +120,62 @@ TEST_F(ShardManagerTesting, CreateShardSuccess) { EXPECT_EQ(_pg_id, shard_info.placement_group); } +TEST_F(ShardManagerTesting, CreateMultiShards) { + auto e = _home_object->shard_manager()->create_shard(_pg_id, Mi).get(); + ASSERT_TRUE(!!e); + homeobject::HSHomeObject* ho = dynamic_cast< homeobject::HSHomeObject* >(_home_object.get()); + auto chunk_num_1 = ho->get_shard_chunk(e.value().id); + ASSERT_TRUE(chunk_num_1.has_value()); + + // create another shard again. + e = _home_object->shard_manager()->create_shard(_pg_id, Mi).get(); + ASSERT_TRUE(!!e); + auto chunk_num_2 = ho->get_shard_chunk(e.value().id); + ASSERT_TRUE(chunk_num_2.has_value()); + + // check if both chunk is on the same pdev; + auto alloc_hint1 = ho->chunk_selector()->chunk_to_hints(chunk_num_1.value()); + auto alloc_hint2 = ho->chunk_selector()->chunk_to_hints(chunk_num_2.value()); + ASSERT_TRUE(alloc_hint1.pdev_id_hint.has_value()); + ASSERT_TRUE(alloc_hint2.pdev_id_hint.has_value()); + ASSERT_TRUE(alloc_hint1.pdev_id_hint.value() == alloc_hint2.pdev_id_hint.value()); +} + +TEST_F(ShardManagerTesting, CreateMultiShardsOnMultiPG) { + // create another PG; + auto peer1 = _home_object->our_uuid(); + auto peer2 = boost::uuids::random_generator()(); + + auto new_pg_id = static_cast< homeobject::pg_id_t >(_pg_id + 1); + auto info = homeobject::PGInfo(_pg_id + 1); + info.members.insert(homeobject::PGMember{peer1, "peer1", 1}); + info.members.insert(homeobject::PGMember{peer2, "peer2", 0}); + EXPECT_TRUE(_home_object->pg_manager()->create_pg(std::move(info)).get()); + + std::vector< homeobject::pg_id_t > pgs{_pg_id, new_pg_id}; + + for (const auto pg : pgs) { + auto e = _home_object->shard_manager()->create_shard(pg, Mi).get(); + ASSERT_TRUE(!!e); + homeobject::HSHomeObject* ho = dynamic_cast< homeobject::HSHomeObject* >(_home_object.get()); + auto chunk_num_1 = ho->get_shard_chunk(e.value().id); + ASSERT_TRUE(chunk_num_1.has_value()); + + // create another shard again. + e = _home_object->shard_manager()->create_shard(_pg_id, Mi).get(); + ASSERT_TRUE(!!e); + auto chunk_num_2 = ho->get_shard_chunk(e.value().id); + ASSERT_TRUE(chunk_num_2.has_value()); + + // check if both chunk is on the same pdev; + auto alloc_hint1 = ho->chunk_selector()->chunk_to_hints(chunk_num_1.value()); + auto alloc_hint2 = ho->chunk_selector()->chunk_to_hints(chunk_num_2.value()); + ASSERT_TRUE(alloc_hint1.pdev_id_hint.has_value()); + ASSERT_TRUE(alloc_hint2.pdev_id_hint.has_value()); + ASSERT_TRUE(alloc_hint1.pdev_id_hint.value() == alloc_hint2.pdev_id_hint.value()); + } +} + TEST_F(ShardManagerTesting, CreateShardAndValidateMembers) { auto e = _home_object->shard_manager()->create_shard(_pg_id, Mi).get(); ASSERT_TRUE(!!e);