From ce58fe994038e3493c2b7de7b5bf232572c6267c Mon Sep 17 00:00:00 2001 From: zilai Date: Tue, 12 Sep 2023 03:29:45 -0700 Subject: [PATCH] fix some review comments --- src/lib/blob_manager.cpp | 8 ++-- src/lib/homeobject_impl.hpp | 20 +++------ src/lib/homestore/homeobject.cpp | 4 +- src/lib/homestore/homeobject.hpp | 9 ++-- src/lib/homestore/shard_manager.cpp | 66 +++++++++++++---------------- src/lib/memory/shard_manager.cpp | 10 ++--- src/lib/pg_manager.cpp | 9 ++++ src/lib/shard_manager.cpp | 17 +++++--- 8 files changed, 72 insertions(+), 71 deletions(-) diff --git a/src/lib/blob_manager.cpp b/src/lib/blob_manager.cpp index 25067f2c..7fb300d8 100644 --- a/src/lib/blob_manager.cpp +++ b/src/lib/blob_manager.cpp @@ -11,7 +11,7 @@ BlobManager::AsyncResult< Blob > HomeObjectImpl::get(shard_id shard, blob_id con uint64_t len) const { return _get_shard(shard).thenValue([this, blob](auto const e) -> BlobManager::Result< Blob > { if (!e) return folly::makeUnexpected(BlobError::UNKNOWN_SHARD); - return _get_blob(e.value(), blob); + return _get_blob(e.value().info, blob); }); } @@ -19,15 +19,15 @@ BlobManager::AsyncResult< blob_id > HomeObjectImpl::put(shard_id shard, Blob&& b return _get_shard(shard).thenValue( [this, blob = std::move(blob)](auto const e) mutable -> BlobManager::Result< blob_id > { if (!e) return folly::makeUnexpected(BlobError::UNKNOWN_SHARD); - if (ShardInfo::State::SEALED == e.value().state) return folly::makeUnexpected(BlobError::INVALID_ARG); - return _put_blob(e.value(), std::move(blob)); + if (ShardInfo::State::SEALED == e.value().info.state) return folly::makeUnexpected(BlobError::INVALID_ARG); + return _put_blob(e.value().info, std::move(blob)); }); } BlobManager::NullAsyncResult HomeObjectImpl::del(shard_id shard, blob_id const& blob) { return _get_shard(shard).thenValue([this, blob](auto const e) mutable -> BlobManager::NullResult { if (!e) return folly::makeUnexpected(BlobError::UNKNOWN_SHARD); - return _del_blob(e.value(), blob); + return _del_blob(e.value().info, blob); }); } diff --git a/src/lib/homeobject_impl.hpp b/src/lib/homeobject_impl.hpp index 6695883d..db38846a 100644 --- a/src/lib/homeobject_impl.hpp +++ b/src/lib/homeobject_impl.hpp @@ -16,24 +16,20 @@ namespace homeobject { inline bool operator<(ShardInfo const& lhs, ShardInfo const& rhs) { return lhs.id < rhs.id; } inline bool operator==(ShardInfo const& lhs, ShardInfo const& rhs) { return lhs.id == rhs.id; } -struct ShardInfoExt { - uint16_t chunk_id; -}; - struct Shard { explicit Shard(ShardInfo info) : info(std::move(info)) {} ShardInfo info; - ShardInfoExt ext_info; + uint16_t chunk_id; }; -using ShardPtr = std::shared_ptr< Shard >; -using ShardPtrList = std::list< ShardPtr >; +using ShardList = std::list< Shard >; +using ShardIterator = ShardList::iterator; struct PG { explicit PG(PGInfo info) : pg_info(std::move(info)) {} PGInfo pg_info; uint64_t shard_sequence_num{0}; - ShardPtrList shards; + ShardList shards; }; class HomeObjectImpl : public HomeObject, @@ -50,8 +46,7 @@ class HomeObjectImpl : public HomeObject, virtual BlobManager::Result< Blob > _get_blob(ShardInfo const&, blob_id) const = 0; virtual BlobManager::NullResult _del_blob(ShardInfo const&, blob_id) = 0; /// - - folly::Future< ShardManager::Result< ShardInfo > > _get_shard(shard_id id) const; + folly::Future< ShardManager::Result< Shard > > _get_shard(shard_id id) const; auto _defer() const { return folly::makeSemiFuture().via(folly::getGlobalCPUExecutor()); } protected: @@ -68,8 +63,9 @@ class HomeObjectImpl : public HomeObject, std::map< pg_id, PG > _pg_map; mutable std::shared_mutex _shard_lock; - std::map < shard_id, ShardPtr > _shard_map; + std::map < shard_id, ShardIterator > _shard_map; /// + PGManager::Result< PG > _get_pg(pg_id pg); public: explicit HomeObjectImpl(std::weak_ptr< HomeObjectApplication >&& application) : _application(std::move(application)) {} @@ -99,8 +95,6 @@ class HomeObjectImpl : public HomeObject, ShardManager::AsyncResult< InfoList > list_shards(pg_id pg) const final; ShardManager::AsyncResult< ShardInfo > seal_shard(shard_id id) final; uint64_t get_current_timestamp(); - - static const std::string s_shard_info_sub_type; /// BlobManager BlobManager::AsyncResult< blob_id > put(shard_id shard, Blob&&) final; BlobManager::AsyncResult< Blob > get(shard_id shard, blob_id const& blob, uint64_t off, uint64_t len) const final; diff --git a/src/lib/homestore/homeobject.cpp b/src/lib/homestore/homeobject.cpp index d919d36b..197012e4 100644 --- a/src/lib/homestore/homeobject.cpp +++ b/src/lib/homestore/homeobject.cpp @@ -87,8 +87,8 @@ void HSHomeObject::on_shard_meta_blk_found(homestore::meta_blk* mblk, sisl::byte std::string shard_info_str; shard_info_str.append(r_cast(buf.bytes()), size); - auto shard = deserialize_shard_info(shard_info_str); - if (shard->info.state == ShardInfo::State::OPEN) { + auto shard = deserialize_shard(shard_info_str); + if (shard.info.state == ShardInfo::State::OPEN) { // create shard; do_commit_new_shard(shard); } else { diff --git a/src/lib/homestore/homeobject.hpp b/src/lib/homestore/homeobject.hpp index a4071f3f..05a63911 100644 --- a/src/lib/homestore/homeobject.hpp +++ b/src/lib/homestore/homeobject.hpp @@ -23,14 +23,13 @@ class HSHomeObject : public HomeObjectImpl { BlobManager::NullResult _del_blob(ShardInfo const&, blob_id) override; /// private: - bool check_if_pg_exist(pg_id pg) const; shard_id generate_new_shard_id(pg_id pg); shard_id make_new_shard_id(pg_id pg, uint64_t sequence_num) const; uint64_t get_sequence_num_from_shard_id(uint64_t shard_id); - std::string serialize_shard_info(ShardPtr shard) const; - ShardPtr deserialize_shard_info(const std::string& shard_info_str) const; - void do_commit_new_shard(ShardPtr shard_info); - void do_commit_seal_shard(ShardPtr shard_info); + std::string serialize_shard(const Shard& shard) const; + Shard deserialize_shard(const std::string& shard_info_str) const; + void do_commit_new_shard(const Shard& shard); + void do_commit_seal_shard(const Shard& shard); public: using HomeObjectImpl::HomeObjectImpl; ~HSHomeObject(); diff --git a/src/lib/homestore/shard_manager.cpp b/src/lib/homestore/shard_manager.cpp index a47e041d..8b66ced9 100644 --- a/src/lib/homestore/shard_manager.cpp +++ b/src/lib/homestore/shard_manager.cpp @@ -9,12 +9,6 @@ namespace homeobject { uint64_t ShardManager::max_shard_size() { return Gi; } uint64_t ShardManager::max_shard_num_in_pg() {return ((uint64_t)0x01) << 48;} -bool HSHomeObject::check_if_pg_exist(pg_id pg) const { - std::scoped_lock lock_guard(_pg_lock); - auto iter = _pg_map.find(pg); - return (iter == _pg_map.end()) ? false : true; -} - shard_id HSHomeObject::generate_new_shard_id(pg_id pg) { std::scoped_lock lock_guard(_pg_lock); auto iter = _pg_map.find(pg); @@ -32,21 +26,21 @@ uint64_t HSHomeObject::get_sequence_num_from_shard_id(uint64_t shard_id) { return shard_id & (0x0000FFFFFFFFFFFF); } -std::string HSHomeObject::serialize_shard_info(ShardPtr shard) const { +std::string HSHomeObject::serialize_shard(const Shard& shard) const { nlohmann::json j; - j["shard_info"]["shard_id"] = shard->info.id; - j["shard_info"]["pg_id"] = shard->info.placement_group; - j["shard_info"]["state"] = shard->info.state; - j["shard_info"]["created_time"] = shard->info.created_time; - j["shard_info"]["modified_time"] = shard->info.last_modified_time; - j["shard_info"]["total_capacity"] = shard->info.total_capacity_bytes; - j["shard_info"]["available_capacity"] = shard->info.available_capacity_bytes; - j["shard_info"]["deleted_capacity"] = shard->info.deleted_capacity_bytes; - j["ext_info"]["chunk_id"] = shard->ext_info.chunk_id; + j["shard_info"]["shard_id"] = shard.info.id; + j["shard_info"]["pg_id"] = shard.info.placement_group; + j["shard_info"]["state"] = shard.info.state; + j["shard_info"]["created_time"] = shard.info.created_time; + j["shard_info"]["modified_time"] = shard.info.last_modified_time; + j["shard_info"]["total_capacity"] = shard.info.total_capacity_bytes; + j["shard_info"]["available_capacity"] = shard.info.available_capacity_bytes; + j["shard_info"]["deleted_capacity"] = shard.info.deleted_capacity_bytes; + j["ext_info"]["chunk_id"] = shard.chunk_id; return j.dump(); } -ShardPtr HSHomeObject::deserialize_shard_info(const std::string& shard_json_str) const { +Shard HSHomeObject::deserialize_shard(const std::string& shard_json_str) const { auto shard_json = nlohmann::json::parse(shard_json_str); ShardInfo shard_info; shard_info.id = shard_json["shard_info"]["shard_id"].get(); @@ -57,18 +51,17 @@ ShardPtr HSHomeObject::deserialize_shard_info(const std::string& shard_json_str) shard_info.available_capacity_bytes = shard_json["shard_info"]["available_capacity"].get(); shard_info.total_capacity_bytes = shard_json["shard_info"]["total_capacity"].get(); shard_info.deleted_capacity_bytes = shard_json["shard_info"]["deleted_capacity"].get(); - auto shard = std::make_shared< Shard >(shard_info); - shard->ext_info.chunk_id = shard_json["ext_info"]["chunk_id"].get(); + auto shard = Shard(shard_info); + shard.chunk_id = shard_json["ext_info"]["chunk_id"].get(); return shard; } ShardManager::Result< ShardInfo > HSHomeObject::_create_shard(pg_id pg_owner, uint64_t size_bytes) { - bool pg_exist = check_if_pg_exist(pg_owner); - if (!pg_exist) { + auto pg = _get_pg(pg_owner); + if (!bool(pg)) { LOGWARN("failed to create shard with non-exist pg [{}]", pg_owner); return folly::makeUnexpected(ShardError::UNKNOWN_PG); } - //TODO: will update to ReplDev when ReplDev on HomeStore is ready; auto replica_set_var = _repl_svc->get_replica_set(fmt::format("{}", pg_owner)); if (std::holds_alternative< home_replication:: ReplServiceError>(replica_set_var)) { @@ -84,9 +77,9 @@ ShardManager::Result< ShardInfo > HSHomeObject::_create_shard(pg_id pg_owner, ui auto new_shard_id = generate_new_shard_id(pg_owner); auto create_time = get_current_timestamp(); - auto shard = std::make_shared(ShardInfo(new_shard_id, pg_owner, ShardInfo::State::OPEN, - create_time, create_time, size_bytes, size_bytes, 0)); - std::string create_shard_message = serialize_shard_info(shard); + auto shard_info = ShardInfo(new_shard_id, pg_owner, ShardInfo::State::OPEN, + create_time, create_time, size_bytes, size_bytes, 0); + std::string create_shard_message = serialize_shard(Shard(shard_info)); //preapre msg header; const uint32_t needed_size = sizeof(ReplicationMessageHeader) + create_shard_message.size(); auto buf = nuraft::buffer::alloc(needed_size); @@ -161,8 +154,8 @@ void HSHomeObject::on_shard_message_commit(int64_t lsn, sisl::blob const& header homestore::hs()->meta_service().add_sub_sb(HSHomeObject::s_shard_info_sub_type, r_cast(string_value.c_str()), string_value.size(), cookie); //update in-memory shard map; - auto shard = deserialize_shard_info(string_value); - if (shard->info.state == ShardInfo::State::OPEN) { + auto shard = deserialize_shard(string_value); + if (shard.info.state == ShardInfo::State::OPEN) { //create shard; do_commit_new_shard(shard); } else { @@ -170,30 +163,31 @@ void HSHomeObject::on_shard_message_commit(int64_t lsn, sisl::blob const& header } if (promise) { - promise->setValue(ShardManager::Result(shard->info)); + promise->setValue(ShardManager::Result(shard.info)); } } -void HSHomeObject::do_commit_new_shard(ShardPtr shard) { +void HSHomeObject::do_commit_new_shard(const Shard& shard) { std::scoped_lock lock_guard(_pg_lock, _shard_lock); - auto pg_iter = _pg_map.find(shard->info.placement_group); + auto pg_iter = _pg_map.find(shard.info.placement_group); RELEASE_ASSERT(pg_iter != _pg_map.end(), "Missing PG info"); - pg_iter->second.shards.push_back(shard); - auto [_, happened] = _shard_map.emplace(shard->info.id, shard); + auto& shards = pg_iter->second.shards; + auto iter = shards.emplace(shards.end(), shard); + auto [_, happened] = _shard_map.emplace(shard.info.id, iter); RELEASE_ASSERT(happened, "duplicated shard info"); //following part is must for follower members or when member is restarted; - auto sequence_num = get_sequence_num_from_shard_id(shard->info.id); + auto sequence_num = get_sequence_num_from_shard_id(shard.info.id); if (sequence_num > pg_iter->second.shard_sequence_num) { pg_iter->second.shard_sequence_num = sequence_num; } } -void HSHomeObject::do_commit_seal_shard(ShardPtr shard) { +void HSHomeObject::do_commit_seal_shard(const Shard& shard) { std::scoped_lock lock_guard(_shard_lock); - auto shard_iter = _shard_map.find(shard->info.id); + auto shard_iter = _shard_map.find(shard.info.id); RELEASE_ASSERT(shard_iter != _shard_map.end(), "Missing shard info"); - *(shard_iter->second) = *shard; + *(shard_iter->second) = shard; } } // namespace homeobject diff --git a/src/lib/memory/shard_manager.cpp b/src/lib/memory/shard_manager.cpp index ddc340d6..2b072ed4 100644 --- a/src/lib/memory/shard_manager.cpp +++ b/src/lib/memory/shard_manager.cpp @@ -16,10 +16,9 @@ ShardManager::Result< ShardInfo > MemoryHomeObject::_create_shard(pg_id pg_owner auto& s_list = pg_it->second.shards; info.id = (((uint64_t)pg_owner) << 48) + s_list.size(); - auto shard = std::make_shared(info); + auto iter = s_list.emplace(s_list.end(), Shard(info)); LOGDEBUG("Creating Shard [{}]: in Pg [{}] of Size [{}b]", info.id, pg_owner, size_bytes); - s_list.push_back(shard); - auto [_, s_happened] = _shard_map.emplace(info.id, shard); + auto [_, s_happened] = _shard_map.emplace(info.id, iter); RELEASE_ASSERT(s_happened, "Duplicate Shard insertion!"); } auto lg = std::scoped_lock(_index_lock); @@ -32,8 +31,9 @@ ShardManager::Result< ShardInfo > MemoryHomeObject::_seal_shard(shard_id id) { auto lg = std::scoped_lock(_shard_lock); auto shard_it = _shard_map.find(id); if (_shard_map.end() == shard_it) return folly::makeUnexpected(ShardError::UNKNOWN_SHARD); - shard_it->second->info.state = ShardInfo::State::SEALED; - return shard_it->second->info; + auto& shard_info = (*shard_it->second).info; + shard_info.state = ShardInfo::State::SEALED; + return shard_info; } } // namespace homeobject diff --git a/src/lib/pg_manager.cpp b/src/lib/pg_manager.cpp index acdd47bb..a74379ee 100644 --- a/src/lib/pg_manager.cpp +++ b/src/lib/pg_manager.cpp @@ -93,4 +93,13 @@ PGManager::NullAsyncResult HomeObjectImpl::replace_member(pg_id id, peer_id cons }); } +PGManager::Result< PG > HomeObjectImpl::_get_pg(pg_id pg) { + std::scoped_lock lock_guard(_pg_lock); + auto iter = _pg_map.find(pg); + if (iter == _pg_map.end()) { + return folly::makeUnexpected(PGError::UNKNOWN_PG); + } + return iter->second; +} + } // namespace homeobject diff --git a/src/lib/shard_manager.cpp b/src/lib/shard_manager.cpp index 458236f6..d83593a6 100644 --- a/src/lib/shard_manager.cpp +++ b/src/lib/shard_manager.cpp @@ -23,8 +23,8 @@ ShardManager::AsyncResult< InfoList > HomeObjectImpl::list_shards(pg_id pg) cons auto info_l = std::list< ShardInfo >(); for (auto const& shard : pg_it->second.shards) { - LOGDEBUG("Listing Shard {}", shard->info.id); - info_l.push_back(shard->info); + LOGDEBUG("Listing Shard {}", shard.info.id); + info_l.push_back(shard.info); } return info_l; }); @@ -36,15 +36,20 @@ ShardManager::AsyncResult< ShardInfo > HomeObjectImpl::seal_shard(shard_id id) { } -ShardManager::AsyncResult< ShardInfo > HomeObjectImpl::get_shard(shard_id id) const { return _get_shard(id); } +ShardManager::AsyncResult< ShardInfo > HomeObjectImpl::get_shard(shard_id id) const { + return _get_shard(id).thenValue([this](auto const e) mutable -> ShardManager::Result< ShardInfo > { + if (!e) { return folly::makeUnexpected(e.error());} + return e.value().info; + }); +} /// // This is used as a first call for many operations and initializes the Future. // -folly::Future< ShardManager::Result< ShardInfo > > HomeObjectImpl::_get_shard(shard_id id) const { - return _defer().thenValue([this, id](auto) -> ShardManager::Result< ShardInfo > { +folly::Future< ShardManager::Result< Shard > > HomeObjectImpl::_get_shard(shard_id id) const { + return _defer().thenValue([this, id](auto) -> ShardManager::Result< Shard > { auto lg = std::shared_lock(_shard_lock); - if (auto it = _shard_map.find(id); _shard_map.end() != it) return it->second->info; + if (auto it = _shard_map.find(id); _shard_map.end() != it) return *(it->second); return folly::makeUnexpected(ShardError::UNKNOWN_SHARD); }); }