From bc51e33d9cd507697b74dae6f68ee6cf42e632cb Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Thu, 23 Apr 2020 13:41:32 -0700 Subject: [PATCH] Make sure (Shared)BlobFileMetaData are owned by shared_ptrs (#6749) Summary: The patch makes a couple of small cleanups to `SharedBlobFileMetaData` and `BlobFileMetaData`: * It makes the constructors private and introduces factory methods to ensure these objects are always owned by `shared_ptr`s. Note that `SharedBlobFileMetaData` has an additional factory that takes a deleter object; we can utilize this to e.g. notify `VersionSet` when a blob file becomes obsolete (which is exactly when `SharedBlobFileMetaData` is destroyed). * It disables move operations explicitly instead of relying on them being suppressed because of a user-declared destructor. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6749 Test Plan: `make check` Reviewed By: riversand963 Differential Revision: D21206947 Pulled By: ltamasi fbshipit-source-id: 9094c14cc335b3e226f883e5a0df4f87a5cdeb95 --- db/blob/blob_file_meta.cc | 4 --- db/blob/blob_file_meta.h | 70 +++++++++++++++++++++++++++----------- db/version_builder.cc | 6 ++-- db/version_builder_test.cc | 4 +-- 4 files changed, 55 insertions(+), 29 deletions(-) diff --git a/db/blob/blob_file_meta.cc b/db/blob/blob_file_meta.cc index ee867298b..14ed81886 100644 --- a/db/blob/blob_file_meta.cc +++ b/db/blob/blob_file_meta.cc @@ -10,10 +10,6 @@ namespace ROCKSDB_NAMESPACE { -SharedBlobFileMetaData::~SharedBlobFileMetaData() { - // TODO: add the blob file to the list of obsolete files here -} - std::string SharedBlobFileMetaData::DebugString() const { std::ostringstream oss; oss << (*this); diff --git a/db/blob/blob_file_meta.h b/db/blob/blob_file_meta.h index 51a2e248b..3acfbb4bc 100644 --- a/db/blob/blob_file_meta.h +++ b/db/blob/blob_file_meta.h @@ -23,22 +23,33 @@ namespace ROCKSDB_NAMESPACE { class SharedBlobFileMetaData { public: - SharedBlobFileMetaData(uint64_t blob_file_number, uint64_t total_blob_count, - uint64_t total_blob_bytes, std::string checksum_method, - std::string checksum_value) - : blob_file_number_(blob_file_number), - total_blob_count_(total_blob_count), - total_blob_bytes_(total_blob_bytes), - checksum_method_(std::move(checksum_method)), - checksum_value_(std::move(checksum_value)) { - assert(checksum_method_.empty() == checksum_value_.empty()); + static std::shared_ptr Create( + uint64_t blob_file_number, uint64_t total_blob_count, + uint64_t total_blob_bytes, std::string checksum_method, + std::string checksum_value) { + return std::shared_ptr(new SharedBlobFileMetaData( + blob_file_number, total_blob_count, total_blob_bytes, + std::move(checksum_method), std::move(checksum_value))); } - ~SharedBlobFileMetaData(); + template + static std::shared_ptr Create( + uint64_t blob_file_number, uint64_t total_blob_count, + uint64_t total_blob_bytes, std::string checksum_method, + std::string checksum_value, Deleter deleter) { + return std::shared_ptr( + new SharedBlobFileMetaData(blob_file_number, total_blob_count, + total_blob_bytes, std::move(checksum_method), + std::move(checksum_value)), + deleter); + } SharedBlobFileMetaData(const SharedBlobFileMetaData&) = delete; SharedBlobFileMetaData& operator=(const SharedBlobFileMetaData&) = delete; + SharedBlobFileMetaData(SharedBlobFileMetaData&&) = delete; + SharedBlobFileMetaData& operator=(SharedBlobFileMetaData&&) = delete; + uint64_t GetBlobFileNumber() const { return blob_file_number_; } uint64_t GetTotalBlobCount() const { return total_blob_count_; } uint64_t GetTotalBlobBytes() const { return total_blob_bytes_; } @@ -48,6 +59,17 @@ class SharedBlobFileMetaData { std::string DebugString() const; private: + SharedBlobFileMetaData(uint64_t blob_file_number, uint64_t total_blob_count, + uint64_t total_blob_bytes, std::string checksum_method, + std::string checksum_value) + : blob_file_number_(blob_file_number), + total_blob_count_(total_blob_count), + total_blob_bytes_(total_blob_bytes), + checksum_method_(std::move(checksum_method)), + checksum_value_(std::move(checksum_value)) { + assert(checksum_method_.empty() == checksum_value_.empty()); + } + uint64_t blob_file_number_; uint64_t total_blob_count_; uint64_t total_blob_bytes_; @@ -68,21 +90,19 @@ std::ostream& operator<<(std::ostream& os, class BlobFileMetaData { public: - BlobFileMetaData(std::shared_ptr shared_meta, - uint64_t garbage_blob_count, uint64_t garbage_blob_bytes) - : shared_meta_(std::move(shared_meta)), - garbage_blob_count_(garbage_blob_count), - garbage_blob_bytes_(garbage_blob_bytes) { - assert(shared_meta_); - assert(garbage_blob_count_ <= shared_meta_->GetTotalBlobCount()); - assert(garbage_blob_bytes_ <= shared_meta_->GetTotalBlobBytes()); + static std::shared_ptr Create( + std::shared_ptr shared_meta, + uint64_t garbage_blob_count, uint64_t garbage_blob_bytes) { + return std::shared_ptr(new BlobFileMetaData( + std::move(shared_meta), garbage_blob_count, garbage_blob_bytes)); } - ~BlobFileMetaData() = default; - BlobFileMetaData(const BlobFileMetaData&) = delete; BlobFileMetaData& operator=(const BlobFileMetaData&) = delete; + BlobFileMetaData(BlobFileMetaData&&) = delete; + BlobFileMetaData& operator=(BlobFileMetaData&&) = delete; + const std::shared_ptr& GetSharedMeta() const { return shared_meta_; } @@ -114,6 +134,16 @@ class BlobFileMetaData { std::string DebugString() const; private: + BlobFileMetaData(std::shared_ptr shared_meta, + uint64_t garbage_blob_count, uint64_t garbage_blob_bytes) + : shared_meta_(std::move(shared_meta)), + garbage_blob_count_(garbage_blob_count), + garbage_blob_bytes_(garbage_blob_bytes) { + assert(shared_meta_); + assert(garbage_blob_count_ <= shared_meta_->GetTotalBlobCount()); + assert(garbage_blob_bytes_ <= shared_meta_->GetTotalBlobBytes()); + } + std::shared_ptr shared_meta_; uint64_t garbage_blob_count_; uint64_t garbage_blob_bytes_; diff --git a/db/version_builder.cc b/db/version_builder.cc index 13b34b3e8..4b609c3b8 100644 --- a/db/version_builder.cc +++ b/db/version_builder.cc @@ -388,7 +388,7 @@ class VersionBuilder::Rep { return Status::Corruption("VersionBuilder", oss.str()); } - auto shared_meta = std::make_shared( + auto shared_meta = SharedBlobFileMetaData::Create( blob_file_number, blob_file_addition.GetTotalBlobCount(), blob_file_addition.GetTotalBlobBytes(), blob_file_addition.GetChecksumMethod(), @@ -396,7 +396,7 @@ class VersionBuilder::Rep { constexpr uint64_t garbage_blob_count = 0; constexpr uint64_t garbage_blob_bytes = 0; - auto new_meta = std::make_shared( + auto new_meta = BlobFileMetaData::Create( std::move(shared_meta), garbage_blob_count, garbage_blob_bytes); changed_blob_files_.emplace(blob_file_number, std::move(new_meta)); @@ -417,7 +417,7 @@ class VersionBuilder::Rep { assert(meta->GetBlobFileNumber() == blob_file_number); - auto new_meta = std::make_shared( + auto new_meta = BlobFileMetaData::Create( meta->GetSharedMeta(), meta->GetGarbageBlobCount() + blob_file_garbage.GetGarbageBlobCount(), meta->GetGarbageBlobBytes() + blob_file_garbage.GetGarbageBlobBytes()); diff --git a/db/version_builder_test.cc b/db/version_builder_test.cc index 9523278f7..a3b06e085 100644 --- a/db/version_builder_test.cc +++ b/db/version_builder_test.cc @@ -82,10 +82,10 @@ class VersionBuilderTest : public testing::Test { uint64_t total_blob_bytes, std::string checksum_method, std::string checksum_value, uint64_t garbage_blob_count, uint64_t garbage_blob_bytes) { - auto shared_meta = std::make_shared( + auto shared_meta = SharedBlobFileMetaData::Create( blob_file_number, total_blob_count, total_blob_bytes, std::move(checksum_method), std::move(checksum_value)); - auto meta = std::make_shared( + auto meta = BlobFileMetaData::Create( std::move(shared_meta), garbage_blob_count, garbage_blob_bytes); vstorage_.AddBlobFile(std::move(meta)); -- GitLab