You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by la...@apache.org on 2023/02/27 16:20:27 UTC
[kudu] branch master updated: [fs] make CommitDeletedBlocks output parameter nullable
This is an automated email from the ASF dual-hosted git repository.
laiyingchun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 5f325d8e1 [fs] make CommitDeletedBlocks output parameter nullable
5f325d8e1 is described below
commit 5f325d8e14e5e22f5978b0325693322fd4847ca8
Author: Yingchun Lai <la...@apache.org>
AuthorDate: Tue Feb 21 10:36:27 2023 +0800
[fs] make CommitDeletedBlocks output parameter nullable
The output parameter of BlockDeletionTransaction::CommitDeletedBlocks
'deleted' is useless in some cases, it could be set to nullptr to
reduce minor filling of the container.
This patch also fixes some issues reported by clang-tidy.
Change-Id: Ia92c36011cf8fc63d58fad7da0e08554b9d5dad6
Reviewed-on: http://gerrit.cloudera.org:8080/19520
Reviewed-by: Yifan Zhang <ch...@163.com>
Reviewed-by: Yuqi Du <sh...@gmail.com>
Reviewed-by: Ashwani Raina <ar...@cloudera.com>
Tested-by: Yingchun Lai <la...@apache.org>
---
src/kudu/fs/block_manager-test.cc | 7 ++--
src/kudu/fs/block_manager.h | 4 +-
src/kudu/fs/file_block_manager.cc | 76 +++++++++++++++++++----------------
src/kudu/fs/log_block_manager-test.cc | 49 +++++++++-------------
src/kudu/fs/log_block_manager.cc | 32 ++++++++++-----
src/kudu/fs/log_block_manager.h | 2 +-
src/kudu/tablet/tablet_metadata.cc | 3 +-
7 files changed, 90 insertions(+), 83 deletions(-)
diff --git a/src/kudu/fs/block_manager-test.cc b/src/kudu/fs/block_manager-test.cc
index 433c5f6e0..90dbe43e6 100644
--- a/src/kudu/fs/block_manager-test.cc
+++ b/src/kudu/fs/block_manager-test.cc
@@ -1189,9 +1189,10 @@ TYPED_TEST(BlockManagerTest, TestBlockTransaction) {
deleted_blocks.clear();
Status s = deletion_transaction->CommitDeletedBlocks(&deleted_blocks);
ASSERT_TRUE(s.IsIOError());
- ASSERT_STR_CONTAINS(s.ToString(), Substitute("only deleted $0 blocks, "
- "first failure",
- deleted_blocks.size()));
+ ASSERT_TRUE(deleted_blocks.empty());
+ ASSERT_STR_MATCHES(s.ToString(),
+ Substitute("only 0/$0 blocks deleted, first failure",
+ created_blocks.size()));
}
} // namespace fs
diff --git a/src/kudu/fs/block_manager.h b/src/kudu/fs/block_manager.h
index e3286b061..aad77adb9 100644
--- a/src/kudu/fs/block_manager.h
+++ b/src/kudu/fs/block_manager.h
@@ -314,8 +314,8 @@ class BlockDeletionTransaction {
// Deletes a group of blocks given the block IDs, the actual deletion will take
// place after the last open reader or writer is closed for each block that needs
// be to deleted. The 'deleted' out parameter will be set with the list of block
- // IDs that were successfully deleted, regardless of the value of returned 'status'
- // is OK or error.
+ // IDs that were successfully deleted if it's not nullptr, regardless of the value
+ // of returned 'status' is OK or error.
//
// Returns the first deletion failure that was seen, if any.
virtual Status CommitDeletedBlocks(std::vector<BlockId>* deleted) = 0;
diff --git a/src/kudu/fs/file_block_manager.cc b/src/kudu/fs/file_block_manager.cc
index ec2648a63..def8b9f0a 100644
--- a/src/kudu/fs/file_block_manager.cc
+++ b/src/kudu/fs/file_block_manager.cc
@@ -25,6 +25,7 @@
#include <ostream>
#include <set>
#include <string>
+#include <type_traits>
#include <utility>
#include <vector>
@@ -232,30 +233,30 @@ void FileBlockLocation::GetAllParentDirs(vector<string>* parent_dirs) const {
// at Close() time. Embedding a FileBlockLocation (and not a simpler
// BlockId) consumes more memory, but the number of outstanding
// FileWritableBlock instances is expected to be low.
-class FileWritableBlock : public WritableBlock {
+class FileWritableBlock final : public WritableBlock {
public:
FileWritableBlock(FileBlockManager* block_manager, FileBlockLocation location,
shared_ptr<WritableFile> writer);
- virtual ~FileWritableBlock();
+ ~FileWritableBlock() override;
- virtual Status Close() OVERRIDE;
+ Status Close() override;
- virtual Status Abort() OVERRIDE;
+ Status Abort() override;
- virtual BlockManager* block_manager() const OVERRIDE;
+ BlockManager* block_manager() const override;
- virtual const BlockId& id() const OVERRIDE;
+ const BlockId& id() const override;
- virtual Status Append(const Slice& data) OVERRIDE;
+ Status Append(const Slice& data) override;
- virtual Status AppendV(ArrayView<const Slice> data) OVERRIDE;
+ Status AppendV(ArrayView<const Slice> data) override;
- virtual Status Finalize() OVERRIDE;
+ Status Finalize() override;
- virtual size_t BytesAppended() const OVERRIDE;
+ size_t BytesAppended() const override;
- virtual State state() const OVERRIDE;
+ State state() const override;
void HandleError(const Status& s) const;
@@ -436,21 +437,21 @@ class FileReadableBlock : public ReadableBlock {
FileReadableBlock(FileBlockManager* block_manager, BlockId block_id,
shared_ptr<RandomAccessFile> reader);
- virtual ~FileReadableBlock();
+ ~FileReadableBlock() override;
- virtual Status Close() OVERRIDE;
+ Status Close() override;
- virtual BlockManager* block_manager() const OVERRIDE;
+ BlockManager* block_manager() const override;
- virtual const BlockId& id() const OVERRIDE;
+ const BlockId& id() const override;
- virtual Status Size(uint64_t* sz) const OVERRIDE;
+ Status Size(uint64_t* sz) const override;
- virtual Status Read(uint64_t offset, Slice result) const OVERRIDE;
+ Status Read(uint64_t offset, Slice result) const override;
- virtual Status ReadV(uint64_t offset, ArrayView<Slice> results) const OVERRIDE;
+ Status ReadV(uint64_t offset, ArrayView<Slice> results) const override;
- virtual size_t memory_footprint() const OVERRIDE;
+ size_t memory_footprint() const override;
void HandleError(const Status& s) const;
@@ -557,18 +558,18 @@ class FileBlockCreationTransaction : public BlockCreationTransaction {
public:
FileBlockCreationTransaction() = default;
- virtual ~FileBlockCreationTransaction() = default;
+ ~FileBlockCreationTransaction() override = default;
- virtual void AddCreatedBlock(std::unique_ptr<WritableBlock> block) override;
+ void AddCreatedBlock(unique_ptr<WritableBlock> block) override;
- virtual Status CommitCreatedBlocks() override;
+ Status CommitCreatedBlocks() override;
private:
- std::vector<std::unique_ptr<FileWritableBlock>> created_blocks_;
+ vector<unique_ptr<FileWritableBlock>> created_blocks_;
};
void FileBlockCreationTransaction::AddCreatedBlock(
- std::unique_ptr<WritableBlock> block) {
+ unique_ptr<WritableBlock> block) {
FileWritableBlock* fwb =
down_cast<FileWritableBlock*>(block.release());
created_blocks_.emplace_back(unique_ptr<FileWritableBlock>(fwb));
@@ -607,16 +608,16 @@ class FileBlockDeletionTransaction : public BlockDeletionTransaction {
: fbm_(fbm) {
}
- virtual ~FileBlockDeletionTransaction() = default;
+ ~FileBlockDeletionTransaction() override = default;
- virtual void AddDeletedBlock(BlockId block) override;
+ void AddDeletedBlock(BlockId block) override;
- virtual Status CommitDeletedBlocks(std::vector<BlockId>* deleted) override;
+ Status CommitDeletedBlocks(vector<BlockId>* deleted) override;
private:
// The owning FileBlockManager. Must outlive the FileBlockDeletionTransaction.
FileBlockManager* fbm_;
- std::vector<BlockId> deleted_blocks_;
+ vector<BlockId> deleted_blocks_;
DISALLOW_COPY_AND_ASSIGN(FileBlockDeletionTransaction);
};
@@ -624,16 +625,22 @@ void FileBlockDeletionTransaction::AddDeletedBlock(BlockId block) {
deleted_blocks_.emplace_back(block);
}
-Status FileBlockDeletionTransaction::CommitDeletedBlocks(std::vector<BlockId>* deleted) {
- deleted->clear();
+Status FileBlockDeletionTransaction::CommitDeletedBlocks(vector<BlockId>* deleted) {
+ if (deleted) {
+ deleted->clear();
+ }
Status first_failure;
+ uint64_t deleted_blocks_count = 0;
for (BlockId block : deleted_blocks_) {
Status s = fbm_->DeleteBlock(block);
// If we get NotFound, then the block was already deleted.
if (!s.ok() && !s.IsNotFound()) {
if (first_failure.ok()) first_failure = s;
} else {
- deleted->emplace_back(block);
+ ++deleted_blocks_count;
+ if (deleted) {
+ deleted->emplace_back(block);
+ }
if (s.ok() && fbm_->metrics_) {
fbm_->metrics_->total_blocks_deleted->Increment();
}
@@ -641,9 +648,10 @@ Status FileBlockDeletionTransaction::CommitDeletedBlocks(std::vector<BlockId>* d
}
if (!first_failure.ok()) {
- first_failure = first_failure.CloneAndPrepend(strings::Substitute("only deleted $0 blocks, "
- "first failure",
- deleted->size()));
+ first_failure = first_failure.CloneAndPrepend(
+ Substitute("only $0/$1 blocks deleted, first failure",
+ deleted_blocks_count,
+ deleted_blocks_.size()));
}
deleted_blocks_.clear();
return first_failure;
diff --git a/src/kudu/fs/log_block_manager-test.cc b/src/kudu/fs/log_block_manager-test.cc
index 2143df510..31149c4e0 100644
--- a/src/kudu/fs/log_block_manager-test.cc
+++ b/src/kudu/fs/log_block_manager-test.cc
@@ -146,9 +146,8 @@ class LogBlockManagerTest : public KuduTest, public ::testing::WithParamInterfac
protected:
LogBlockManager* CreateBlockManager(const scoped_refptr<MetricEntity>& metric_entity,
- std::vector<std::string> test_data_dirs = {}) {
+ vector<string> test_data_dirs = {}) {
PrepareDataDirs(&test_data_dirs);
-
if (!dd_manager_) {
// Ensure the directory manager is initialized.
CHECK_OK(DataDirManager::CreateNewForTests(env_, test_data_dirs,
@@ -164,7 +163,7 @@ class LogBlockManagerTest : public KuduTest, public ::testing::WithParamInterfac
Status ReopenBlockManager(const scoped_refptr<MetricEntity>& metric_entity = nullptr,
FsReport* report = nullptr,
- std::vector<std::string> test_data_dirs = {},
+ vector<string> test_data_dirs = {},
bool force = false) {
PrepareDataDirs(&test_data_dirs);
@@ -300,7 +299,8 @@ class LogBlockManagerTest : public KuduTest, public ::testing::WithParamInterfac
break;
}
}
- void PrepareDataDirs(std::vector<std::string>* test_data_dirs) {
+
+ void PrepareDataDirs(vector<string>* test_data_dirs) {
if (test_data_dirs->empty()) {
*test_data_dirs = { test_dir_ };
}
@@ -444,8 +444,7 @@ TEST_P(LogBlockManagerTest, MetricsTest) {
shared_ptr<BlockDeletionTransaction> deletion_transaction =
bm_->NewDeletionTransaction();
deletion_transaction->AddDeletedBlock(saved_id);
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
NO_FATALS(CheckLogMetrics(new_entity,
{ {9 * 1024, &METRIC_log_block_manager_bytes_under_management},
{10, &METRIC_log_block_manager_blocks_under_management},
@@ -627,8 +626,7 @@ TEST_P(LogBlockManagerTest, TestReuseBlockIds) {
for (const BlockId& b : block_ids) {
deletion_transaction->AddDeletedBlock(b);
}
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
// Reset the block ID sequence and re-create new blocks which should reuse the same
@@ -737,8 +735,7 @@ TEST_P(LogBlockManagerTest, TestMetadataTruncation) {
shared_ptr<BlockDeletionTransaction> deletion_transaction =
bm_->NewDeletionTransaction();
deletion_transaction->AddDeletedBlock(created_blocks[0]);
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
ASSERT_OK(bm_->GetAllBlockIds(&block_ids));
ASSERT_EQ(3, block_ids.size());
@@ -987,8 +984,7 @@ TEST_P(LogBlockManagerTest, TestContainerWithManyHoles) {
for (int i = 0; i < ids.size(); i += 2) {
deletion_transaction->AddDeletedBlock(ids[i]);
}
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
// Delete all of the blocks belonging to the interior node. If KUDU-1508
// applies, this should corrupt the filesystem.
@@ -997,7 +993,7 @@ TEST_P(LogBlockManagerTest, TestContainerWithManyHoles) {
for (int i = 1; i < last_interior_node_block_number; i += 2) {
deletion_transaction->AddDeletedBlock(ids[i]);
}
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
TEST_P(LogBlockManagerTest, TestParseKernelRelease) {
@@ -1111,8 +1107,7 @@ TEST_P(LogBlockManagerStartupBenchmarkTest, StartupBenchmark) {
break;
}
}
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
}
@@ -1299,8 +1294,7 @@ TEST_F(LogBlockManagerTest, TestContainerBlockLimitingByMetadataSizeWithCompacti
}
deletion_transaction->AddDeletedBlock(id);
}
- vector<BlockId> deleted;
- RETURN_NOT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ RETURN_NOT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
return Status::OK();
};
@@ -1432,8 +1426,7 @@ TEST_P(LogBlockManagerTest, TestMisalignedBlocksFuzz) {
deletion_transaction->AddDeletedBlock(id);
}
}
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
// Wait for the block manager to punch out all of the holes. It's easiest to
@@ -1745,8 +1738,7 @@ TEST_P(LogBlockManagerTest, TestDeleteDeadContainersAtStartup) {
shared_ptr<BlockDeletionTransaction> deletion_transaction =
this->bm_->NewDeletionTransaction();
deletion_transaction->AddDeletedBlock(block_id);
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
ASSERT_OK(ReopenBlockManager());
ASSERT_FALSE(env_->FileExists(data_file_name));
@@ -1787,8 +1779,7 @@ TEST_P(LogBlockManagerTest, TestCompactFullContainerMetadataAtStartup) {
shared_ptr<BlockDeletionTransaction> deletion_transaction =
bm_->NewDeletionTransaction();
deletion_transaction->AddDeletedBlock(id);
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
num_blocks_deleted++;
FsReport report;
@@ -1847,8 +1838,7 @@ TEST_P(LogBlockManagerTest, TestDeleteFromContainerAfterMetadataCompaction) {
block_ids.emplace_back(block->id());
}
}
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
// Reopen the block manager. This will cause it to compact all of the metadata
@@ -1869,8 +1859,7 @@ TEST_P(LogBlockManagerTest, TestDeleteFromContainerAfterMetadataCompaction) {
for (const BlockId &b : block_ids) {
deletion_transaction->AddDeletedBlock(b);
}
- vector<BlockId> deleted;
- ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr));
}
// Reopen to make sure that the metadata can be properly loaded and
@@ -2162,12 +2151,11 @@ TEST_P(LogBlockManagerTest, TestDoNotDeleteFakeDeadContainer) {
// Delete the bunch of blocks.
{
- vector<BlockId> deleted;
shared_ptr<BlockDeletionTransaction> transaction = bm_->NewDeletionTransaction();
for (const auto& e : blocks) {
transaction->AddDeletedBlock(e);
}
- ASSERT_OK(transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(transaction->CommitDeletedBlocks(nullptr));
transaction.reset();
for (const auto& data_dir : dd_manager_->dirs()) {
data_dir->WaitOnClosures();
@@ -2243,10 +2231,9 @@ TEST_P(LogBlockManagerTest, TestHalfPresentContainer) {
};
const auto DeleteBlock = [&] () {
- vector<BlockId> deleted;
shared_ptr<BlockDeletionTransaction> transaction = bm_->NewDeletionTransaction();
transaction->AddDeletedBlock(block_id);
- ASSERT_OK(transaction->CommitDeletedBlocks(&deleted));
+ ASSERT_OK(transaction->CommitDeletedBlocks(nullptr));
transaction.reset();
for (const auto& data_dir : dd_manager_->dirs()) {
data_dir->WaitOnClosures();
diff --git a/src/kudu/fs/log_block_manager.cc b/src/kudu/fs/log_block_manager.cc
index 2f9639344..8f71b98bb 100644
--- a/src/kudu/fs/log_block_manager.cc
+++ b/src/kudu/fs/log_block_manager.cc
@@ -342,7 +342,7 @@ class LogBlock : public RefCountedThreadSafe<LogBlock> {
//
// There's no reference to a LogBlock as this block has yet to be
// persisted.
-class LogWritableBlock : public WritableBlock {
+class LogWritableBlock final : public WritableBlock {
public:
LogWritableBlock(LogBlockContainerRefPtr container, BlockId block_id,
int64_t block_offset);
@@ -1743,7 +1743,9 @@ LogBlockDeletionTransaction::~LogBlockDeletionTransaction() {
}
Status LogBlockDeletionTransaction::CommitDeletedBlocks(vector<BlockId>* deleted) {
- deleted->clear();
+ if (deleted) {
+ deleted->clear();
+ }
shared_ptr<LogBlockDeletionTransaction> transaction = shared_from_this();
vector<LogBlockRefPtr> log_blocks;
@@ -1760,8 +1762,10 @@ Status LogBlockDeletionTransaction::CommitDeletedBlocks(vector<BlockId>* deleted
}
if (!first_failure.ok()) {
- first_failure = first_failure.CloneAndPrepend(Substitute("only deleted $0 blocks, "
- "first failure", deleted->size()));
+ first_failure = first_failure.CloneAndPrepend(
+ Substitute("only $0/$1 blocks deleted, first failure",
+ log_blocks.size(),
+ deleted_blocks_.size()));
}
deleted_blocks_.clear();
return first_failure;
@@ -1928,8 +1932,7 @@ Status LogWritableBlock::Abort() {
shared_ptr<BlockDeletionTransaction> deletion_transaction =
container_->block_manager()->NewDeletionTransaction();
deletion_transaction->AddDeletedBlock(id());
- vector<BlockId> deleted;
- return deletion_transaction->CommitDeletedBlocks(&deleted);
+ return deletion_transaction->CommitDeletedBlocks(nullptr);
}
const BlockId& LogWritableBlock::id() const {
@@ -2653,12 +2656,17 @@ bool LogBlockManager::AddLogBlock(LogBlockRefPtr lb) {
return true;
}
-Status LogBlockManager::RemoveLogBlocks(vector<BlockId> block_ids,
+Status LogBlockManager::RemoveLogBlocks(const vector<BlockId>& block_ids,
vector<LogBlockRefPtr>* log_blocks,
vector<BlockId>* deleted) {
+ DCHECK(log_blocks);
Status first_failure;
vector<LogBlockRefPtr> lbs;
- int64_t malloc_space = 0, blocks_length = 0;
+ if (deleted) {
+ deleted->reserve(block_ids.size());
+ }
+ int64_t malloc_space = 0;
+ int64_t blocks_length = 0;
for (const auto& block_id : block_ids) {
LogBlockRefPtr lb;
Status s = RemoveLogBlock(block_id, &lb);
@@ -2671,7 +2679,9 @@ Status LogBlockManager::RemoveLogBlocks(vector<BlockId> block_ids,
} else {
// If we get NotFound, then the block was already deleted.
DCHECK(s.IsNotFound());
- deleted->emplace_back(block_id);
+ if (deleted) {
+ deleted->emplace_back(block_id);
+ }
}
}
@@ -2718,7 +2728,9 @@ Status LogBlockManager::RemoveLogBlocks(vector<BlockId> block_ids,
});
}
- deleted->emplace_back(lb->block_id());
+ if (deleted) {
+ deleted->emplace_back(lb->block_id());
+ }
log_blocks->emplace_back(std::move(lb));
}
}
diff --git a/src/kudu/fs/log_block_manager.h b/src/kudu/fs/log_block_manager.h
index 828b0d9de..0e5fffab8 100644
--- a/src/kudu/fs/log_block_manager.h
+++ b/src/kudu/fs/log_block_manager.h
@@ -313,7 +313,7 @@ class LogBlockManager : public BlockManager {
// blocks were already deleted, e.g encountered 'NotFound' error during removal.
//
// Returns the first deletion failure that was seen, if any.
- Status RemoveLogBlocks(std::vector<BlockId> block_ids,
+ Status RemoveLogBlocks(const std::vector<BlockId>& block_ids,
std::vector<LogBlockRefPtr>* log_blocks,
std::vector<BlockId>* deleted);
diff --git a/src/kudu/tablet/tablet_metadata.cc b/src/kudu/tablet/tablet_metadata.cc
index d50002c92..0c8065227 100644
--- a/src/kudu/tablet/tablet_metadata.cc
+++ b/src/kudu/tablet/tablet_metadata.cc
@@ -574,8 +574,7 @@ void TabletMetadata::DeleteOrphanedBlocks(const BlockIdContainer& blocks) {
for (const BlockId& b : blocks) {
transaction->AddDeletedBlock(b);
}
- vector<BlockId> deleted;
- WARN_NOT_OK(transaction->CommitDeletedBlocks(&deleted),
+ WARN_NOT_OK(transaction->CommitDeletedBlocks(nullptr),
"not all orphaned blocks were deleted");
// Regardless of whether we deleted all the blocks or not, remove them from