kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: KUDU-2055 [part 4]: refactor BM to remove BlockManager::DeleteBlock
Date Thu, 12 Oct 2017 18:27:01 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 9583cad6e -> f373af07d


KUDU-2055 [part 4]: refactor BM to remove BlockManager::DeleteBlock

This patch refactors block manager to use fs::BlockDeletionTransaction
instead of BlockManager::DeleteBlock for block deletions, in order to
simplify the interfaces of block manager and have a uniform path for
block deletions.

Change-Id: Ied2ab44f33bce29706f8f41acc3d468582edad6e
Reviewed-on: http://gerrit.cloudera.org:8080/8219
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <adar@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/f373af07
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f373af07
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f373af07

Branch: refs/heads/master
Commit: f373af07da4995872d37c6817f214bc39f9d8e4d
Parents: 9583cad
Author: hahao <hao.hao@cloudera.com>
Authored: Tue Oct 10 17:53:16 2017 -0700
Committer: Dan Burkert <danburkert@apache.org>
Committed: Thu Oct 12 18:26:49 2017 +0000

----------------------------------------------------------------------
 src/kudu/fs/block_manager-stress-test.cc        |  9 ++-
 src/kudu/fs/block_manager-test.cc               | 32 ++++++++---
 src/kudu/fs/block_manager.h                     |  8 ---
 src/kudu/fs/file_block_manager.cc               |  3 +-
 src/kudu/fs/file_block_manager.h                | 12 +++-
 src/kudu/fs/fs_manager.cc                       |  6 --
 src/kudu/fs/log_block_manager-test.cc           | 60 +++++++++++++++-----
 src/kudu/fs/log_block_manager.cc                |  1 +
 src/kudu/fs/log_block_manager.h                 | 13 ++++-
 src/kudu/tablet/tablet_metadata.cc              | 20 +++----
 src/kudu/tools/kudu-tool-test.cc                |  7 ++-
 src/kudu/tools/tool_action_fs.cc                | 23 ++++++--
 .../tserver/tablet_copy_source_session-test.cc  |  8 ++-
 13 files changed, 140 insertions(+), 62 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/block_manager-stress-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager-stress-test.cc b/src/kudu/fs/block_manager-stress-test.cc
index 53c10d7..06be125 100644
--- a/src/kudu/fs/block_manager-stress-test.cc
+++ b/src/kudu/fs/block_manager-stress-test.cc
@@ -88,6 +88,7 @@ DEFINE_string(block_manager_paths, "", "Comma-separated list of paths to
"
               "test path");
 
 using std::string;
+using std::shared_ptr;
 using std::unique_ptr;
 using std::unordered_map;
 using std::vector;
@@ -448,6 +449,8 @@ void BlockManagerStressTest<T>::DeleterThread() {
   while (!ShouldStop(tight_loop)) {
     // Grab a block at random.
     BlockId to_delete;
+    shared_ptr<BlockDeletionTransaction> deletion_transaction =
+        this->bm_->NewDeletionTransaction();
     {
       std::unique_lock<simple_spinlock> l(lock_);
       // If we only have a small number of live blocks, don't delete any.
@@ -472,8 +475,10 @@ void BlockManagerStressTest<T>::DeleterThread() {
     }
 
     // And delete it.
-    CHECK_OK(bm_->DeleteBlock(to_delete));
-    num_blocks_deleted++;
+    deletion_transaction->AddDeletedBlock(to_delete);
+    vector<BlockId> deleted;
+    CHECK_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+    num_blocks_deleted += deleted.size();
   }
 
   total_blocks_deleted_.IncrementBy(num_blocks_deleted);

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/block_manager-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager-test.cc b/src/kudu/fs/block_manager-test.cc
index 552a533..68a71df 100644
--- a/src/kudu/fs/block_manager-test.cc
+++ b/src/kudu/fs/block_manager-test.cc
@@ -40,6 +40,7 @@
 #include "kudu/fs/fs.pb.h"
 #include "kudu/fs/fs_report.h"
 #include "kudu/fs/log_block_manager.h"
+#include "kudu/gutil/basictypes.h"
 #include "kudu/gutil/bind.h"
 #include "kudu/gutil/casts.h"
 #include "kudu/gutil/gscoped_ptr.h"
@@ -456,7 +457,12 @@ TYPED_TEST(BlockManagerTest, EndToEndTest) {
   LOG(INFO) << "Block memory footprint: " << read_block->memory_footprint();
 
   // Delete the block.
-  ASSERT_OK(this->bm_->DeleteBlock(written_block->id()));
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      this->bm_->NewDeletionTransaction();
+  deletion_transaction->AddDeletedBlock(written_block->id());
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+  ASSERT_EQ(1, deleted.size());
   ASSERT_TRUE(this->bm_->OpenBlock(written_block->id(), nullptr)
               .IsNotFound());
 }
@@ -495,7 +501,12 @@ TYPED_TEST(BlockManagerTest, ReadAfterDeleteTest) {
   // Open it for reading, then delete it. Subsequent opens should fail.
   unique_ptr<ReadableBlock> read_block;
   ASSERT_OK(this->bm_->OpenBlock(written_block->id(), &read_block));
-  ASSERT_OK(this->bm_->DeleteBlock(written_block->id()));
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      this->bm_->NewDeletionTransaction();
+  deletion_transaction->AddDeletedBlock(written_block->id());
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+  ASSERT_EQ(1, deleted.size());
   ASSERT_TRUE(this->bm_->OpenBlock(written_block->id(), nullptr)
               .IsNotFound());
 
@@ -655,7 +666,12 @@ TYPED_TEST(BlockManagerTest, PersistenceTest) {
   ASSERT_OK(this->bm_->CreateBlock(this->test_block_opts_, &written_block3));
   ASSERT_OK(written_block3->Append(test_data));
   ASSERT_OK(written_block3->Close());
-  ASSERT_OK(this->bm_->DeleteBlock(written_block3->id()));
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      this->bm_->NewDeletionTransaction();
+  deletion_transaction->AddDeletedBlock(written_block3->id());
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+  ASSERT_EQ(1, deleted.size());
 
   // Reopen the block manager. This may read block metadata from disk.
   //
@@ -940,15 +956,14 @@ TYPED_TEST(BlockManagerTest, TestMetadataOkayDespiteFailure) {
 
     int num_deleted = 0;
     int num_deleted_attempts = 0;
+    shared_ptr<BlockDeletionTransaction> deletion_transaction =
+        this->bm_->NewDeletionTransaction();
     for (auto it = ids.begin(); it != ids.end();) {
       // TODO(adar): the lbm removes a block from its block map even if the
       // on-disk deletion fails. When that's fixed, update this code to
       // erase() only if s.ok().
-      Status s = this->bm_->DeleteBlock(*it);
+      deletion_transaction->AddDeletedBlock(*it);
       it = ids.erase(it);
-      if (s.ok()) {
-        num_deleted++;
-      }
       num_deleted_attempts++;
 
       // Skip every other block.
@@ -956,6 +971,9 @@ TYPED_TEST(BlockManagerTest, TestMetadataOkayDespiteFailure) {
         it++;
       }
     }
+    vector<BlockId> deleted;
+    ignore_result(deletion_transaction->CommitDeletedBlocks(&deleted));
+    num_deleted += deleted.size();
     LOG(INFO) << Substitute("Successfully deleted $0 blocks on $1 attempts",
                             num_deleted, num_deleted_attempts);
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/block_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager.h b/src/kudu/fs/block_manager.h
index 641479e..cbe36e0 100644
--- a/src/kudu/fs/block_manager.h
+++ b/src/kudu/fs/block_manager.h
@@ -247,14 +247,6 @@ class BlockManager {
   virtual Status OpenBlock(const BlockId& block_id,
                            std::unique_ptr<ReadableBlock>* block) = 0;
 
-  // Deletes an existing block, allowing its space to be reclaimed by the
-  // filesystem. The change is immediately made durable.
-  //
-  // Blocks may be deleted while they are open for reading or writing;
-  // the actual deletion will take place after the last open reader or
-  // writer is closed.
-  virtual Status DeleteBlock(const BlockId& block_id) = 0;
-
   // Constructs a block creation transaction to group a set of block creation
   // operations and closes the registered blocks together.
   virtual std::unique_ptr<BlockCreationTransaction> NewCreationTransaction() = 0;

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/file_block_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/file_block_manager.cc b/src/kudu/fs/file_block_manager.cc
index b3e6dbe..6694b88 100644
--- a/src/kudu/fs/file_block_manager.cc
+++ b/src/kudu/fs/file_block_manager.cc
@@ -319,7 +319,7 @@ Status FileWritableBlock::Close() {
 
 Status FileWritableBlock::Abort() {
   RETURN_NOT_OK(Close(NO_SYNC));
-  return block_manager()->DeleteBlock(id());
+  return block_manager_->DeleteBlock(id());
 }
 
 BlockManager* FileWritableBlock::block_manager() const {
@@ -612,6 +612,7 @@ void FileBlockDeletionTransaction::AddDeletedBlock(BlockId block) {
 }
 
 Status FileBlockDeletionTransaction::CommitDeletedBlocks(std::vector<BlockId>* deleted)
{
+  deleted->clear();
   Status first_failure;
   for (BlockId block : deleted_blocks_) {
     Status s = fbm_->DeleteBlock(block);

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/file_block_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/fs/file_block_manager.h b/src/kudu/fs/file_block_manager.h
index fd3535b..bc114a1 100644
--- a/src/kudu/fs/file_block_manager.h
+++ b/src/kudu/fs/file_block_manager.h
@@ -45,6 +45,7 @@ class FsErrorManager;
 struct FsReport;
 
 namespace internal {
+class FileBlockDeletionTransaction;
 class FileBlockLocation;
 class FileReadableBlock;
 class FileWritableBlock;
@@ -86,8 +87,6 @@ class FileBlockManager : public BlockManager {
   Status OpenBlock(const BlockId& block_id,
                    std::unique_ptr<ReadableBlock>* block) override;
 
-  Status DeleteBlock(const BlockId& block_id) override;
-
   std::unique_ptr<BlockCreationTransaction> NewCreationTransaction() override;
 
   std::shared_ptr<BlockDeletionTransaction> NewDeletionTransaction() override;
@@ -97,10 +96,19 @@ class FileBlockManager : public BlockManager {
   FsErrorManager* error_manager() override { return error_manager_; }
 
  private:
+  friend class internal::FileBlockDeletionTransaction;
   friend class internal::FileBlockLocation;
   friend class internal::FileReadableBlock;
   friend class internal::FileWritableBlock;
 
+  // Deletes an existing block, allowing its space to be reclaimed by the
+  // filesystem. The change is immediately made durable.
+  //
+  // Blocks may be deleted while they are open for reading or writing;
+  // the actual deletion will take place after the last open reader or
+  // writer is closed.
+  Status DeleteBlock(const BlockId& block_id);
+
   // Synchronizes the metadata for a block with the given location.
   Status SyncMetadata(const internal::FileBlockLocation& location);
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/fs_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/fs_manager.cc b/src/kudu/fs/fs_manager.cc
index a0c78c6..deeb52a 100644
--- a/src/kudu/fs/fs_manager.cc
+++ b/src/kudu/fs/fs_manager.cc
@@ -649,12 +649,6 @@ Status FsManager::OpenBlock(const BlockId& block_id, unique_ptr<ReadableBlock>*
   return block_manager_->OpenBlock(block_id, block);
 }
 
-Status FsManager::DeleteBlock(const BlockId& block_id) {
-  CHECK(!read_only_);
-
-  return block_manager_->DeleteBlock(block_id);
-}
-
 bool FsManager::BlockExists(const BlockId& block_id) const {
   unique_ptr<ReadableBlock> block;
   return block_manager_->OpenBlock(block_id, &block).ok();

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/log_block_manager-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/log_block_manager-test.cc b/src/kudu/fs/log_block_manager-test.cc
index b3f76f7..5dc5099 100644
--- a/src/kudu/fs/log_block_manager-test.cc
+++ b/src/kudu/fs/log_block_manager-test.cc
@@ -66,6 +66,7 @@
 using kudu::pb_util::ReadablePBContainerFile;
 using std::set;
 using std::string;
+using std::shared_ptr;
 using std::unique_ptr;
 using std::unordered_map;
 using std::unordered_set;
@@ -320,7 +321,11 @@ TEST_F(LogBlockManagerTest, MetricsTest) {
   ASSERT_NO_FATAL_FAILURE(CheckLogMetrics(new_entity, 10 * 1024, 11, 10, 10));
 
   // Delete a block. Its contents should no longer be under management.
-  ASSERT_OK(bm_->DeleteBlock(saved_id));
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      bm_->NewDeletionTransaction();
+  deletion_transaction->AddDeletedBlock(saved_id);
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
   ASSERT_NO_FATAL_FAILURE(CheckLogMetrics(new_entity, 9 * 1024, 10, 10, 10));
 }
 
@@ -727,17 +732,22 @@ TEST_F(LogBlockManagerTest, TestContainerWithManyHoles) {
   // in the container by forcing the filesystem to alternate every hole with
   // a live extent.
   LOG(INFO) << "Deleting every other block";
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      this->bm_->NewDeletionTransaction();
   for (int i = 0; i < ids.size(); i += 2) {
-    ASSERT_OK(bm_->DeleteBlock(ids[i]));
+    deletion_transaction->AddDeletedBlock(ids[i]);
   }
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
 
   // Delete all of the blocks belonging to the interior node. If KUDU-1508
   // applies, this should corrupt the filesystem.
   LOG(INFO) << Substitute("Deleting remaining blocks up to block number $0",
                           last_interior_node_block_number);
   for (int i = 1; i < last_interior_node_block_number; i += 2) {
-    ASSERT_OK(bm_->DeleteBlock(ids[i]));
+    deletion_transaction->AddDeletedBlock(ids[i]);
   }
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
 }
 
 TEST_F(LogBlockManagerTest, TestParseKernelRelease) {
@@ -932,14 +942,18 @@ TEST_F(LogBlockManagerTest, TestMisalignedBlocksFuzz) {
     ASSERT_EQ(container_name, mb.container);
   }
 
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      this->bm_->NewDeletionTransaction();
   // Delete about half of them, chosen randomly.
   vector<BlockId> block_ids;
   ASSERT_OK(bm_->GetAllBlockIds(&block_ids));
   for (const auto& id : block_ids) {
     if (rand() % 2) {
-      ASSERT_OK(bm_->DeleteBlock(id));
+      deletion_transaction->AddDeletedBlock(id);
     }
   }
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
 
   // Wait for the block manager to punch out all of the holes. It's easiest to
   // do this by reopening it; shutdown will wait for outstanding hole punches.
@@ -1235,7 +1249,11 @@ TEST_F(LogBlockManagerTest, TestDeleteDeadContainersAtStartup) {
 
   // Delete the one block and reopen it again. The container files should have
   // been deleted.
-  ASSERT_OK(bm_->DeleteBlock(block->id()));
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      this->bm_->NewDeletionTransaction();
+  deletion_transaction->AddDeletedBlock(block->id());
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
   ASSERT_OK(ReopenBlockManager());
   ASSERT_FALSE(env_->FileExists(data_file_name));
   ASSERT_FALSE(env_->FileExists(metadata_file_name));
@@ -1316,15 +1334,21 @@ TEST_F(LogBlockManagerTest, TestDeleteFromContainerAfterMetadataCompaction)
{
 
   // Create many container with a bunch of blocks, half of which are deleted.
   vector<BlockId> block_ids;
-  for (int i = 0; i < 1000; i++) {
-    unique_ptr<WritableBlock> block;
-    ASSERT_OK(bm_->CreateBlock(test_block_opts_, &block));
-    ASSERT_OK(block->Close());
-    if (i % 2 == 1) {
-      ASSERT_OK(bm_->DeleteBlock(block->id()));
-    } else {
-      block_ids.emplace_back(block->id());
+  {
+    shared_ptr<BlockDeletionTransaction> deletion_transaction =
+        this->bm_->NewDeletionTransaction();
+    for (int i = 0; i < 1000; i++) {
+      unique_ptr<WritableBlock> block;
+      ASSERT_OK(bm_->CreateBlock(test_block_opts_, &block));
+      ASSERT_OK(block->Close());
+      if (i % 2 == 1) {
+        deletion_transaction->AddDeletedBlock(block->id());
+      } else {
+        block_ids.emplace_back(block->id());
+      }
     }
+    vector<BlockId> deleted;
+    ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
   }
 
   // Reopen the block manager. This will cause it to compact all of the metadata
@@ -1338,8 +1362,14 @@ TEST_F(LogBlockManagerTest, TestDeleteFromContainerAfterMetadataCompaction)
{
   // we have file_cache capacity, this will also generate a mix of cache hits,
   // misses, and re-insertions.
   std::random_shuffle(block_ids.begin(), block_ids.end());
-  for (const BlockId& b : block_ids) {
-    ASSERT_OK(bm_->DeleteBlock(b));
+  {
+    shared_ptr<BlockDeletionTransaction> deletion_transaction =
+        this->bm_->NewDeletionTransaction();
+    for (const BlockId &b : block_ids) {
+      deletion_transaction->AddDeletedBlock(b);
+    }
+    vector<BlockId> deleted;
+    ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
   }
 
   // Reopen to make sure that the metadata can be properly loaded and

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/log_block_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/log_block_manager.cc b/src/kudu/fs/log_block_manager.cc
index dcb0e32..5f3d365 100644
--- a/src/kudu/fs/log_block_manager.cc
+++ b/src/kudu/fs/log_block_manager.cc
@@ -1208,6 +1208,7 @@ void LogBlockDeletionTransaction::AddDeletedBlock(BlockId block) {
 }
 
 Status LogBlockDeletionTransaction::CommitDeletedBlocks(std::vector<BlockId>* deleted)
{
+  deleted->clear();
   Status first_failure;
   for (BlockId block : deleted_blocks_) {
     Status s = lbm_->DeleteBlock(block);

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/fs/log_block_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/fs/log_block_manager.h b/src/kudu/fs/log_block_manager.h
index ea4d4ae..4b0f7b2 100644
--- a/src/kudu/fs/log_block_manager.h
+++ b/src/kudu/fs/log_block_manager.h
@@ -57,6 +57,7 @@ struct FsReport;
 namespace internal {
 class LogBlock;
 class LogBlockContainer;
+class LogBlockDeletionTransaction;
 class LogWritableBlock;
 
 struct LogBlockManagerMetrics;
@@ -178,8 +179,6 @@ class LogBlockManager : public BlockManager {
   Status OpenBlock(const BlockId& block_id,
                    std::unique_ptr<ReadableBlock>* block) override;
 
-  Status DeleteBlock(const BlockId& block_id) override;
-
   std::unique_ptr<BlockCreationTransaction> NewCreationTransaction() override;
 
   std::shared_ptr<BlockDeletionTransaction> NewDeletionTransaction() override;
@@ -191,6 +190,7 @@ class LogBlockManager : public BlockManager {
  private:
   FRIEND_TEST(LogBlockManagerTest, TestAbortBlock);
   FRIEND_TEST(LogBlockManagerTest, TestCloseFinalizedBlock);
+  FRIEND_TEST(LogBlockManagerTest, TestCompactFullContainerMetadataAtStartup);
   FRIEND_TEST(LogBlockManagerTest, TestFinalizeBlock);
   FRIEND_TEST(LogBlockManagerTest, TestLIFOContainerSelection);
   FRIEND_TEST(LogBlockManagerTest, TestLookupBlockLimit);
@@ -199,6 +199,7 @@ class LogBlockManager : public BlockManager {
   FRIEND_TEST(LogBlockManagerTest, TestReuseBlockIds);
 
   friend class internal::LogBlockContainer;
+  friend class internal::LogBlockDeletionTransaction;
   friend class internal::LogWritableBlock;
 
   // Type for the actual block map used to store all live blocks.
@@ -237,6 +238,14 @@ class LogBlockManager : public BlockManager {
       std::string,
       std::vector<BlockRecordPB>> BlockRecordsByContainerMap;
 
+  // Deletes an existing block, allowing its space to be reclaimed by the
+  // filesystem. The change is immediately made durable.
+  //
+  // Blocks may be deleted while they are open for reading or writing;
+  // the actual deletion will take place after the last open reader or
+  // writer is closed.
+  Status DeleteBlock(const BlockId& block_id);
+
   // Adds an as of yet unseen container to this block manager.
   //
   // Must be called with 'lock_' held.

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/tablet/tablet_metadata.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_metadata.cc b/src/kudu/tablet/tablet_metadata.cc
index ff1904a..873eb10 100644
--- a/src/kudu/tablet/tablet_metadata.cc
+++ b/src/kudu/tablet/tablet_metadata.cc
@@ -31,6 +31,7 @@
 #include "kudu/consensus/opid.pb.h"
 #include "kudu/consensus/opid_util.h"
 #include "kudu/fs/block_id.h"
+#include "kudu/fs/block_manager.h"
 #include "kudu/fs/data_dirs.h"
 #include "kudu/fs/fs.pb.h"
 #include "kudu/fs/fs_manager.h"
@@ -61,6 +62,8 @@ TAG_FLAG(enable_tablet_orphaned_block_deletion, runtime);
 using base::subtle::Barrier_AtomicIncrement;
 using kudu::consensus::MinimumOpId;
 using kudu::consensus::OpId;
+using kudu::fs::BlockManager;
+using kudu::fs::BlockDeletionTransaction;
 using kudu::pb_util::SecureDebugString;
 using kudu::pb_util::SecureShortDebugString;
 using std::memory_order_relaxed;
@@ -465,19 +468,14 @@ void TabletMetadata::DeleteOrphanedBlocks(const vector<BlockId>&
blocks) {
     return;
   }
 
-  vector<BlockId> deleted;
+  BlockManager* bm = fs_manager()->block_manager();
+  shared_ptr<BlockDeletionTransaction> transaction = bm->NewDeletionTransaction();
   for (const BlockId& b : blocks) {
-    Status s = fs_manager()->DeleteBlock(b);
-    // If we get NotFound, then the block was actually successfully
-    // deleted before. So, we can remove it from our orphaned block list
-    // as if it was a success.
-    if (!s.ok() && !s.IsNotFound()) {
-      WARN_NOT_OK(s, Substitute("Could not delete block $0", b.ToString()));
-      continue;
-    }
-
-    deleted.push_back(b);
+    transaction->AddDeletedBlock(b);
   }
+  vector<BlockId> deleted;
+  WARN_NOT_OK(transaction->CommitDeletedBlocks(&deleted),
+              "not all orphaned blocks were deleted");
 
   // Remove the successfully-deleted blocks from the set.
   {

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/tools/kudu-tool-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index 7590d32..28054cb 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -134,6 +134,7 @@ using consensus::OpId;
 using consensus::RECEIVED_OPID;
 using consensus::ReplicateRefPtr;
 using consensus::ReplicateMsg;
+using fs::BlockDeletionTransaction;
 using fs::FsReport;
 using fs::WritableBlock;
 using itest::ExternalMiniClusterFsInspector;
@@ -624,10 +625,12 @@ TEST_F(ToolTest, TestFsCheck) {
     FsManager fs(env_, kTestDir);
     FsReport report;
     ASSERT_OK(fs.Open(&report));
+    std::shared_ptr<BlockDeletionTransaction> deletion_transaction =
+        fs.block_manager()->NewDeletionTransaction();
     for (int i = 0; i < block_ids.size(); i += 2) {
-      ASSERT_OK(fs.DeleteBlock(block_ids[i]));
-      missing_ids.push_back(block_ids[i]);
+      deletion_transaction->AddDeletedBlock(block_ids[i]);
     }
+    deletion_transaction->CommitDeletedBlocks(&missing_ids);
   }
   NO_FATALS(RunFsCheck(Substitute("fs check --fs_wal_dir=$0", kTestDir),
                        block_ids.size() / 2, kTabletId, missing_ids, 0));

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/tools/tool_action_fs.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_action_fs.cc b/src/kudu/tools/tool_action_fs.cc
index 139387e..444b62e 100644
--- a/src/kudu/tools/tool_action_fs.cc
+++ b/src/kudu/tools/tool_action_fs.cc
@@ -65,11 +65,13 @@ namespace tools {
 using cfile::CFileReader;
 using cfile::CFileIterator;
 using cfile::ReaderOptions;
+using fs::BlockDeletionTransaction;
 using fs::FsReport;
 using fs::ReadableBlock;
 using std::cout;
 using std::endl;
 using std::string;
+using std::shared_ptr;
 using std::unique_ptr;
 using std::unordered_map;
 using std::vector;
@@ -139,6 +141,11 @@ Status Check(const RunnerContext& /*context*/) {
 
   // Add orphaned blocks to the report after attempting to repair them.
   report.orphaned_block_check.emplace();
+  shared_ptr<BlockDeletionTransaction> deletion_transaction;
+  if (FLAGS_repair) {
+    deletion_transaction = fs_manager.block_manager()->NewDeletionTransaction();
+  }
+  vector<BlockId> deleted;
   for (const auto& id : orphaned_block_ids) {
     // Opening a block isn't free, but the number of orphaned blocks shouldn't
     // be extraordinarily high.
@@ -151,14 +158,20 @@ Status Check(const RunnerContext& /*context*/) {
     fs::OrphanedBlockCheck::Entry entry(id, size);
 
     if (FLAGS_repair) {
-      Status s = fs_manager.DeleteBlock(id);
-      WARN_NOT_OK(s, "Could not delete orphaned block");
-      if (s.ok()) {
-        entry.repaired = true;
-      }
+      deletion_transaction->AddDeletedBlock(id);
     }
     report.orphaned_block_check->entries.emplace_back(entry);
   }
+
+  if (FLAGS_repair) {
+    WARN_NOT_OK(deletion_transaction->CommitDeletedBlocks(&deleted),
+                "Could not delete orphaned blocks");
+    BlockIdSet deleted_set(deleted.begin(), deleted.end());
+    for (auto& entry : report.orphaned_block_check->entries) {
+      if (ContainsKey(deleted_set, entry.block_id)) entry.repaired = true;
+    }
+  }
+
   return report.PrintAndCheckForFatalErrors();
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/f373af07/src/kudu/tserver/tablet_copy_source_session-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_source_session-test.cc b/src/kudu/tserver/tablet_copy_source_session-test.cc
index f0cd053..2447805 100644
--- a/src/kudu/tserver/tablet_copy_source_session-test.cc
+++ b/src/kudu/tserver/tablet_copy_source_session-test.cc
@@ -91,6 +91,7 @@ using consensus::ConsensusMetadataManager;
 using consensus::RaftConfigPB;
 using consensus::RaftPeerPB;
 using consensus::kMinimumTerm;
+using fs::BlockDeletionTransaction;
 using fs::ReadableBlock;
 using log::Log;
 using log::LogOptions;
@@ -362,9 +363,14 @@ TEST_F(TabletCopyTest, TestBlocksAreFetchableAfterBeingDeleted) {
   }
 
   // Delete them.
+  shared_ptr<BlockDeletionTransaction> deletion_transaction =
+      fs_manager()->block_manager()->NewDeletionTransaction();
   for (const BlockId& block_id : data_blocks) {
-    ASSERT_OK(fs_manager()->DeleteBlock(block_id));
+    deletion_transaction->AddDeletedBlock(block_id);
   }
+  vector<BlockId> deleted;
+  ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
+  ASSERT_EQ(data_blocks.size(), deleted.size());
 
   // Read them back.
   for (const BlockId& block_id : data_blocks) {


Mime
View raw message