kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject [2/2] kudu git commit: KUDU-1853 (redux). tablet copy: Don't orphan blocks on failure
Date Thu, 04 May 2017 20:47:24 GMT
KUDU-1853 (redux). tablet copy: Don't orphan blocks on failure

Previously, if a tablet copy failed we would orphan data blocks. This
patch makes it so that a failed tablet copy operation that does not
involve a process crash does not orphan data blocks.

This also refactors some deletion logic out of TSTabletManager so that
TabletCopyClient will tombstone partially-copied tablets when the copy
operation fails.

This version of the patch addresses and tests for a data loss issue
(KUDU-1968) that was merged along with a previous version of this patch,
and released with Kudu 1.3.0, before it was reverted in Kudu 1.3.1.

Additional changes in the revised patch:

* Don't check that block ids do not overlap between source and
  destination in tablet_copy_client-test since that's not guaranteed.
* Attempt to detect data loss in tablet_copy_client-test and fail the
  test if detected. In the case of the log block manager, this check is
  currently not reliable due to KUDU-1980.
* Rename old_superblock_ to remote_superblock_ in
  TabletCopyClientSession.

Originally committed as 72541b47eb55b2df4eab5d6050f517476ed6d370 before
being reverted due to KUDU-1968.

Change-Id: Ic999ccd27859ace0d635255e86729dff8e1d8349
Reviewed-on: http://gerrit.cloudera.org:8080/6733
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/5ae27d73
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/5ae27d73
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/5ae27d73

Branch: refs/heads/master
Commit: 5ae27d733fed878f4a8c110e93ef7e57bb48f30e
Parents: 89806c6
Author: Mike Percy <mpercy@apache.org>
Authored: Wed Jan 25 20:10:19 2017 -0800
Committer: Mike Percy <mpercy@apache.org>
Committed: Thu May 4 20:44:46 2017 +0000

----------------------------------------------------------------------
 .../integration-tests/delete_table-itest.cc     |   4 +-
 src/kudu/tserver/tablet_copy-test-base.h        |  34 ++-
 src/kudu/tserver/tablet_copy_client-test.cc     | 192 ++++++++++++-----
 src/kudu/tserver/tablet_copy_client.cc          | 210 ++++++++++++-------
 src/kudu/tserver/tablet_copy_client.h           |  53 +++--
 src/kudu/tserver/tablet_service.cc              |   9 +-
 src/kudu/tserver/ts_tablet_manager.cc           |  69 +++---
 src/kudu/tserver/ts_tablet_manager.h            |   7 -
 8 files changed, 374 insertions(+), 204 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/integration-tests/delete_table-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/delete_table-itest.cc b/src/kudu/integration-tests/delete_table-itest.cc
index 73f58ff..e0b0d95 100644
--- a/src/kudu/integration-tests/delete_table-itest.cc
+++ b/src/kudu/integration-tests/delete_table-itest.cc
@@ -652,8 +652,8 @@ TEST_F(DeleteTableITest, TestAutoTombstoneAfterTabletCopyRemoteFails) {
   {
     vector<ListTabletsResponsePB::StatusAndSchemaPB> status_pbs;
     ASSERT_OK(WaitForNumTabletsOnTS(ts, 1, kTimeout, &status_pbs));
-    ASSERT_STR_CONTAINS(status_pbs[0].tablet_status().last_status(),
-                        "Tombstoned tablet: Tablet Copy: Unable to fetch data from remote peer");
+    ASSERT_STR_MATCHES(status_pbs[0].tablet_status().last_status(),
+                       "Tablet Copy: Tombstoned tablet .*: Tablet copy aborted");
   }
 
   // Now bring the other replicas back, re-elect the previous leader (TS-1),

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/tablet_copy-test-base.h
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy-test-base.h b/src/kudu/tserver/tablet_copy-test-base.h
index 2d5fc9a..bb9bc45 100644
--- a/src/kudu/tserver/tablet_copy-test-base.h
+++ b/src/kudu/tserver/tablet_copy-test-base.h
@@ -20,6 +20,7 @@
 #include "kudu/tserver/tablet_server-test-base.h"
 
 #include <string>
+#include <vector>
 
 #include "kudu/consensus/log_anchor_registry.h"
 #include "kudu/consensus/opid_util.h"
@@ -34,8 +35,6 @@
 namespace kudu {
 namespace tserver {
 
-using consensus::MinimumOpId;
-
 // Number of times to roll the log.
 static const int kNumLogRolls = 2;
 
@@ -49,7 +48,7 @@ class TabletCopyTest : public TabletServerTestBase {
     // Flush(), Log GC is allowed to eat the logs before we get around to
     // starting a tablet copy session.
     tablet_peer_->log_anchor_registry()->Register(
-      MinimumOpId().index(), CURRENT_TEST_NAME(), &anchor_);
+        consensus::MinimumOpId().index(), CURRENT_TEST_NAME(), &anchor_);
     NO_FATALS(GenerateTestData());
   }
 
@@ -61,10 +60,35 @@ class TabletCopyTest : public TabletServerTestBase {
  protected:
   // Grab the first column block we find in the SuperBlock.
   static BlockId FirstColumnBlockId(const tablet::TabletSuperBlockPB& superblock) {
+    DCHECK_GT(superblock.rowsets_size(), 0);
     const tablet::RowSetDataPB& rowset = superblock.rowsets(0);
+    DCHECK_GT(rowset.columns_size(), 0);
     const tablet::ColumnDataPB& column = rowset.columns(0);
-    const BlockIdPB& block_id_pb = column.block();
-    return BlockId::FromPB(block_id_pb);
+    return BlockId::FromPB(column.block());
+  }
+
+  // Return a vector of the blocks contained in the specified superblock (not
+  // including orphaned blocks).
+  static vector<BlockId> ListBlocks(const tablet::TabletSuperBlockPB& superblock) {
+    vector<BlockId> block_ids;
+    for (const auto& rowset : superblock.rowsets()) {
+      for (const auto& col : rowset.columns()) {
+        block_ids.emplace_back(col.block().id());
+      }
+      for (const auto& redos : rowset.redo_deltas()) {
+        block_ids.emplace_back(redos.block().id());
+      }
+      for (const auto& undos : rowset.undo_deltas()) {
+        block_ids.emplace_back(undos.block().id());
+      }
+      if (rowset.has_bloom_block()) {
+        block_ids.emplace_back(rowset.bloom_block().id());
+      }
+      if (rowset.has_adhoc_index_block()) {
+        block_ids.emplace_back(rowset.adhoc_index_block().id());
+      }
+    }
+    return block_ids;
   }
 
   // Check that the contents and CRC32C of a DataChunkPB are equal to a local buffer.

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/tablet_copy_client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_client-test.cc b/src/kudu/tserver/tablet_copy_client-test.cc
index b775c58..cf6cdd5 100644
--- a/src/kudu/tserver/tablet_copy_client-test.cc
+++ b/src/kudu/tserver/tablet_copy_client-test.cc
@@ -16,6 +16,10 @@
 // under the License.
 #include "kudu/tserver/tablet_copy-test-base.h"
 
+#include <tuple>
+
+#include <glog/stl_logging.h>
+
 #include "kudu/consensus/quorum_util.h"
 #include "kudu/gutil/strings/fastmem.h"
 #include "kudu/tablet/tablet_bootstrap.h"
@@ -29,6 +33,7 @@ namespace tserver {
 
 using consensus::GetRaftConfigLeader;
 using consensus::RaftPeerPB;
+using std::tuple;
 using std::unique_ptr;
 using tablet::TabletMetadata;
 
@@ -44,8 +49,8 @@ class TabletCopyClientTest : public TabletCopyTest {
     tablet_peer_->WaitUntilConsensusRunning(MonoDelta::FromSeconds(10.0));
     rpc::MessengerBuilder(CURRENT_TEST_NAME()).Build(&messenger_);
     client_.reset(new TabletCopyClient(GetTabletId(),
-                                            fs_manager_.get(),
-                                            messenger_));
+                                       fs_manager_.get(),
+                                       messenger_));
     ASSERT_OK(GetRaftConfigLeader(tablet_peer_->consensus()
         ->ConsensusState(consensus::CONSENSUS_CONFIG_COMMITTED), &leader_));
 
@@ -91,6 +96,12 @@ Status TabletCopyClientTest::CompareFileContents(const string& path1, const stri
   return Status::OK();
 }
 
+// Implementation test that no blocks exist in the new superblock before fetching.
+TEST_F(TabletCopyClientTest, TestNoBlocksAtStart) {
+  ASSERT_GT(ListBlocks(*client_->remote_superblock_).size(), 0);
+  ASSERT_EQ(0, ListBlocks(*client_->superblock_).size());
+}
+
 // Basic begin / end tablet copy session.
 TEST_F(TabletCopyClientTest, TestBeginEndSession) {
   ASSERT_OK(client_->FetchAll(nullptr /* no listener */));
@@ -99,7 +110,7 @@ TEST_F(TabletCopyClientTest, TestBeginEndSession) {
 
 // Basic data block download unit test.
 TEST_F(TabletCopyClientTest, TestDownloadBlock) {
-  BlockId block_id = FirstColumnBlockId(*client_->superblock_);
+  BlockId block_id = FirstColumnBlockId(*client_->remote_superblock_);
   Slice slice;
   faststring scratch;
 
@@ -113,8 +124,6 @@ TEST_F(TabletCopyClientTest, TestDownloadBlock) {
   ASSERT_OK(client_->DownloadBlock(block_id, &new_block_id));
 
   // Ensure it placed the block where we expected it to.
-  s = ReadLocalBlockFile(fs_manager_.get(), block_id, &scratch, &slice);
-  ASSERT_TRUE(s.IsNotFound()) << "Expected block not found: " << s.ToString();
   ASSERT_OK(ReadLocalBlockFile(fs_manager_.get(), new_block_id, &scratch, &slice));
 }
 
@@ -174,67 +183,140 @@ TEST_F(TabletCopyClientTest, TestVerifyData) {
   LOG(INFO) << "Expected error returned: " << s.ToString();
 }
 
-namespace {
-
-vector<BlockId> GetAllSortedBlocks(const tablet::TabletSuperBlockPB& sb) {
-  vector<BlockId> data_blocks;
-
-  for (const tablet::RowSetDataPB& rowset : sb.rowsets()) {
-    for (const tablet::DeltaDataPB& redo : rowset.redo_deltas()) {
-      data_blocks.push_back(BlockId::FromPB(redo.block()));
-    }
-    for (const tablet::DeltaDataPB& undo : rowset.undo_deltas()) {
-      data_blocks.push_back(BlockId::FromPB(undo.block()));
-    }
-    for (const tablet::ColumnDataPB& column : rowset.columns()) {
-      data_blocks.push_back(BlockId::FromPB(column.block()));
-    }
-    if (rowset.has_bloom_block()) {
-      data_blocks.push_back(BlockId::FromPB(rowset.bloom_block()));
-    }
-    if (rowset.has_adhoc_index_block()) {
-      data_blocks.push_back(BlockId::FromPB(rowset.adhoc_index_block()));
-    }
-  }
-
-  std::sort(data_blocks.begin(), data_blocks.end(), BlockIdCompare());
-  return data_blocks;
-}
-
-} // anonymous namespace
-
 TEST_F(TabletCopyClientTest, TestDownloadAllBlocks) {
   // Download all the blocks.
   ASSERT_OK(client_->DownloadBlocks());
 
-  // Verify that the new superblock reflects the changes in block IDs.
-  //
-  // As long as block IDs are generated with UUIDs or something equally
-  // unique, there's no danger of a block in the new superblock somehow
-  // being assigned the same ID as a block in the existing superblock.
-  vector<BlockId> old_data_blocks = GetAllSortedBlocks(*client_->superblock_.get());
-  vector<BlockId> new_data_blocks = GetAllSortedBlocks(*client_->new_superblock_.get());
-  vector<BlockId> result;
-  std::set_intersection(old_data_blocks.begin(), old_data_blocks.end(),
-                        new_data_blocks.begin(), new_data_blocks.end(),
-                        std::back_inserter(result), BlockIdCompare());
-  ASSERT_TRUE(result.empty());
+  // After downloading blocks, verify that the old and remote and local
+  // superblock point to the same number of blocks.
+  vector<BlockId> old_data_blocks = ListBlocks(*client_->remote_superblock_);
+  vector<BlockId> new_data_blocks = ListBlocks(*client_->superblock_);
   ASSERT_EQ(old_data_blocks.size(), new_data_blocks.size());
 
-  // Verify that the old blocks aren't found. We're using a different
-  // FsManager than 'tablet_peer', so the only way an old block could end
-  // up in ours is due to a tablet copy client bug.
-  for (const BlockId& block_id : old_data_blocks) {
-    unique_ptr<fs::ReadableBlock> block;
-    Status s = fs_manager_->OpenBlock(block_id, &block);
-    ASSERT_TRUE(s.IsNotFound()) << "Expected block not found: " << s.ToString();
-  }
-  // And the new blocks are all present.
+  // Verify that the new blocks are all present.
   for (const BlockId& block_id : new_data_blocks) {
     unique_ptr<fs::ReadableBlock> block;
     ASSERT_OK(fs_manager_->OpenBlock(block_id, &block));
   }
 }
 
+enum DownloadBlocks {
+  kDownloadBlocks,    // Fetch blocks from remote.
+  kNoDownloadBlocks,  // Do not fetch blocks from remote.
+};
+enum DeleteTrigger {
+  kAbortMethod, // Delete data via Abort().
+  kDestructor,  // Delete data via destructor.
+  kNoDelete     // Don't delete data.
+};
+
+struct AbortTestParams {
+  DownloadBlocks download_blocks;
+  DeleteTrigger delete_type;
+};
+
+class TabletCopyClientAbortTest : public TabletCopyClientTest,
+                                  public ::testing::WithParamInterface<
+                                      tuple<DownloadBlocks, DeleteTrigger>> {
+ protected:
+  // Create the specified number of blocks with junk data for testing purposes.
+  void CreateTestBlocks(int num_blocks);
+};
+
+INSTANTIATE_TEST_CASE_P(BlockDeleteTriggers,
+                        TabletCopyClientAbortTest,
+                        ::testing::Combine(
+                            ::testing::Values(kDownloadBlocks, kNoDownloadBlocks),
+                            ::testing::Values(kAbortMethod, kDestructor, kNoDelete)));
+
+void TabletCopyClientAbortTest::CreateTestBlocks(int num_blocks) {
+  for (int i = 0; i < num_blocks; i++) {
+    unique_ptr<fs::WritableBlock> block;
+    ASSERT_OK(fs_manager_->CreateNewBlock(&block));
+    block->Append("Test");
+    ASSERT_OK(block->Close());
+  }
+}
+
+// Test that we can clean up our downloaded blocks either explicitly using
+// Abort() or implicitly by destroying the TabletCopyClient instance before
+// calling Finish(). Also ensure that no data loss occurs.
+TEST_P(TabletCopyClientAbortTest, TestAbort) {
+  tuple<DownloadBlocks, DeleteTrigger> param = GetParam();
+  DownloadBlocks download_blocks = std::get<0>(param);
+  DeleteTrigger trigger = std::get<1>(param);
+
+  // Check that there are remote blocks.
+  vector<BlockId> remote_block_ids = ListBlocks(*client_->remote_superblock_);
+  ASSERT_FALSE(remote_block_ids.empty());
+  int num_remote_blocks = client_->CountRemoteBlocks();
+  ASSERT_GT(num_remote_blocks, 0);
+  ASSERT_EQ(num_remote_blocks, remote_block_ids.size());
+
+  // Create some local blocks so we can check that we didn't lose any existing
+  // data on abort. TODO(mpercy): The data loss check here will likely never
+  // trigger until we fix KUDU-1980 because there is a workaround / hack in the
+  // LBM that randomizes the starting block id for each BlockManager instance.
+  // Therefore the block ids will never overlap.
+  const int kNumBlocksToCreate = 100;
+  NO_FATALS(CreateTestBlocks(kNumBlocksToCreate));
+
+  vector<BlockId> local_block_ids;
+  ASSERT_OK(fs_manager_->block_manager()->GetAllBlockIds(&local_block_ids));
+  ASSERT_EQ(kNumBlocksToCreate, local_block_ids.size());
+  VLOG(1) << "Local blocks: " << local_block_ids;
+
+  int num_blocks_downloaded = 0;
+  if (download_blocks == kDownloadBlocks) {
+    ASSERT_OK(client_->DownloadBlocks());
+    num_blocks_downloaded = num_remote_blocks;
+  }
+
+  vector<BlockId> new_local_block_ids;
+  ASSERT_OK(fs_manager_->block_manager()->GetAllBlockIds(&new_local_block_ids));
+  ASSERT_EQ(kNumBlocksToCreate + num_blocks_downloaded, new_local_block_ids.size());
+
+  // Download a WAL segment.
+  ASSERT_OK(fs_manager_->CreateDirIfMissing(fs_manager_->GetTabletWalDir(GetTabletId())));
+  uint64_t seqno = client_->wal_seqnos_[0];
+  ASSERT_OK(client_->DownloadWAL(seqno));
+  string wal_path = fs_manager_->GetWalSegmentFileName(GetTabletId(), seqno);
+  ASSERT_TRUE(fs_manager_->Exists(wal_path));
+
+  scoped_refptr<TabletMetadata> meta = client_->meta_;
+
+  switch (trigger) {
+    case kAbortMethod:
+      ASSERT_OK(client_->Abort());
+      break;
+    case kDestructor:
+      client_.reset();
+      break;
+    case kNoDelete:
+      // Call Finish() and then destroy the object.
+      // It should not delete its downloaded blocks.
+      ASSERT_OK(client_->Finish());
+      client_.reset();
+      break;
+    default:
+      FAIL();
+  }
+
+  if (trigger == kNoDelete) {
+    vector<BlockId> new_local_block_ids;
+    ASSERT_OK(fs_manager_->block_manager()->GetAllBlockIds(&new_local_block_ids));
+    ASSERT_EQ(kNumBlocksToCreate + num_blocks_downloaded, new_local_block_ids.size());
+  } else {
+    ASSERT_EQ(tablet::TABLET_DATA_TOMBSTONED, meta->tablet_data_state());
+    ASSERT_FALSE(fs_manager_->Exists(wal_path));
+    vector<BlockId> latest_blocks;
+    fs_manager_->block_manager()->GetAllBlockIds(&latest_blocks);
+    ASSERT_EQ(local_block_ids.size(), latest_blocks.size());
+  }
+  for (const auto& block_id : local_block_ids) {
+    ASSERT_TRUE(fs_manager_->BlockExists(block_id)) << "Missing block: " << block_id;
+  }
+}
+
 } // namespace tserver
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/tablet_copy_client.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_client.cc b/src/kudu/tserver/tablet_copy_client.cc
index 856ee75..495074f 100644
--- a/src/kudu/tserver/tablet_copy_client.cc
+++ b/src/kudu/tserver/tablet_copy_client.cc
@@ -38,6 +38,7 @@
 #include "kudu/tserver/tablet_copy.pb.h"
 #include "kudu/tserver/tablet_copy.proxy.h"
 #include "kudu/tserver/tablet_server.h"
+#include "kudu/tserver/ts_tablet_manager.h"
 #include "kudu/util/crc.h"
 #include "kudu/util/env.h"
 #include "kudu/util/env_util.h"
@@ -95,14 +96,12 @@ using tablet::TabletStatusListener;
 using tablet::TabletSuperBlockPB;
 
 TabletCopyClient::TabletCopyClient(std::string tablet_id,
-                                             FsManager* fs_manager,
-                                             shared_ptr<Messenger> messenger)
+                                   FsManager* fs_manager,
+                                   shared_ptr<Messenger> messenger)
     : tablet_id_(std::move(tablet_id)),
       fs_manager_(fs_manager),
       messenger_(std::move(messenger)),
-      started_(false),
-      downloaded_wal_(false),
-      downloaded_blocks_(false),
+      state_(kInitialized),
       replace_tombstoned_tablet_(false),
       status_listener_(nullptr),
       session_idle_timeout_millis_(0),
@@ -110,11 +109,14 @@ TabletCopyClient::TabletCopyClient(std::string tablet_id,
 
 TabletCopyClient::~TabletCopyClient() {
   // Note: Ending the tablet copy session releases anchors on the remote.
-  WARN_NOT_OK(EndRemoteSession(), "Unable to close tablet copy session");
+  WARN_NOT_OK(EndRemoteSession(), Substitute("$0Unable to close tablet copy session",
+                                             LogPrefix()));
+  WARN_NOT_OK(Abort(), Substitute("$0Failed to fully clean up tablet after aborted copy",
+                                  LogPrefix()));
 }
 
 Status TabletCopyClient::SetTabletToReplace(const scoped_refptr<TabletMetadata>& meta,
-                                                 int64_t caller_term) {
+                                            int64_t caller_term) {
   CHECK_EQ(tablet_id_, meta->tablet_id());
   TabletDataState data_state = meta->tablet_data_state();
   if (data_state != tablet::TABLET_DATA_TOMBSTONED) {
@@ -150,8 +152,8 @@ Status TabletCopyClient::SetTabletToReplace(const scoped_refptr<TabletMetadata>&
 }
 
 Status TabletCopyClient::Start(const HostPort& copy_source_addr,
-                                    scoped_refptr<TabletMetadata>* meta) {
-  CHECK(!started_);
+                               scoped_refptr<TabletMetadata>* meta) {
+  CHECK_EQ(kInitialized, state_);
   start_time_micros_ = GetCurrentTimeMicros();
 
   Sockaddr addr;
@@ -192,8 +194,24 @@ Status TabletCopyClient::Start(const HostPort& copy_source_addr,
 
   session_id_ = resp.session_id();
   session_idle_timeout_millis_ = resp.session_idle_timeout_millis();
-  superblock_.reset(resp.release_superblock());
+
+  // Store a copy of the remote (old) superblock.
+  remote_superblock_.reset(resp.release_superblock());
+
+  // Make a copy of the remote superblock. We first clear out the remote blocks
+  // from this structure and then add them back in as they are downloaded.
+  superblock_.reset(new TabletSuperBlockPB(*remote_superblock_));
+
+  // The block ids (in active rowsets as well as from orphaned blocks) on the
+  // remote have no meaning to us and could cause data loss if accidentally
+  // deleted locally. We must clear them all.
+  superblock_->clear_rowsets();
+  superblock_->clear_orphaned_blocks();
+
+  // Set the data state to COPYING to indicate that, on crash, this replica
+  // should be discarded.
   superblock_->set_tablet_data_state(tablet::TABLET_DATA_COPYING);
+
   wal_seqnos_.assign(resp.wal_segment_seqnos().begin(), resp.wal_segment_seqnos().end());
   remote_committed_cstate_.reset(resp.release_initial_committed_cstate());
 
@@ -212,19 +230,19 @@ Status TabletCopyClient::Start(const HostPort& copy_source_addr,
       return Status::InvalidArgument(
           Substitute("Tablet $0: source peer has term $1 but "
                      "tombstoned replica has last-logged opid with higher term $2. "
-                      "Refusing tablet copy from source peer $3",
-                      tablet_id_,
-                      remote_committed_cstate_->current_term(),
-                      last_logged_term,
-                      copy_peer_uuid));
+                     "Refusing tablet copy from source peer $3",
+                     tablet_id_,
+                     remote_committed_cstate_->current_term(),
+                     last_logged_term,
+                     copy_peer_uuid));
     }
 
-    // Remove any existing orphaned blocks from the tablet, and
+    // Remove any existing orphaned blocks and WALs from the tablet, and
     // set the data state to 'COPYING'.
-    RETURN_NOT_OK_PREPEND(meta_->DeleteTabletData(tablet::TABLET_DATA_COPYING, boost::none),
-                          "Couldn't replace superblock with COPYING data state");
+    RETURN_NOT_OK_PREPEND(
+        TSTabletManager::DeleteTabletData(meta_, tablet::TABLET_DATA_COPYING, boost::none),
+        "Could not replace superblock with COPYING data state");
   } else {
-
     Partition partition;
     Partition::FromPB(superblock_->partition(), &partition);
     PartitionSchema partition_schema;
@@ -242,7 +260,7 @@ Status TabletCopyClient::Start(const HostPort& copy_source_addr,
                                             &meta_));
   }
 
-  started_ = true;
+  state_ = kStarted;
   if (meta) {
     *meta = meta_;
   }
@@ -250,7 +268,8 @@ Status TabletCopyClient::Start(const HostPort& copy_source_addr,
 }
 
 Status TabletCopyClient::FetchAll(TabletStatusListener* status_listener) {
-  CHECK(started_);
+  CHECK_EQ(kStarted, state_);
+
   status_listener_ = status_listener;
 
   // Download all the files (serially, for now, but in parallel in the future).
@@ -262,9 +281,8 @@ Status TabletCopyClient::FetchAll(TabletStatusListener* status_listener) {
 
 Status TabletCopyClient::Finish() {
   CHECK(meta_);
-  CHECK(started_);
-  CHECK(downloaded_wal_);
-  CHECK(downloaded_blocks_);
+  CHECK_EQ(kStarted, state_);
+  state_ = kFinished;
 
   RETURN_NOT_OK(WriteConsensusMetadata());
 
@@ -273,8 +291,8 @@ Status TabletCopyClient::Finish() {
   // superblock is in a valid state to bootstrap from.
   LOG_WITH_PREFIX(INFO) << "Tablet Copy complete. Replacing tablet superblock.";
   UpdateStatusMessage("Replacing tablet superblock");
-  new_superblock_->set_tablet_data_state(tablet::TABLET_DATA_READY);
-  RETURN_NOT_OK(meta_->ReplaceSuperBlock(*new_superblock_));
+  superblock_->set_tablet_data_state(tablet::TABLET_DATA_READY);
+  RETURN_NOT_OK(meta_->ReplaceSuperBlock(*superblock_));
 
   if (FLAGS_tablet_copy_save_downloaded_metadata) {
     string meta_path = fs_manager_->GetTabletMetadataPath(tablet_id_);
@@ -287,6 +305,27 @@ Status TabletCopyClient::Finish() {
   return Status::OK();
 }
 
+Status TabletCopyClient::Abort() {
+  if (state_ != kStarted) {
+    return Status::OK();
+  }
+  state_ = kFinished;
+  CHECK(meta_);
+
+  // Write the in-progress superblock to disk so that when we delete the tablet
+  // data all the partial blocks we have persisted will be deleted.
+  DCHECK_EQ(tablet::TABLET_DATA_COPYING, superblock_->tablet_data_state());
+  RETURN_NOT_OK(meta_->ReplaceSuperBlock(*superblock_));
+
+  // Delete all of the tablet data, including blocks and WALs.
+  RETURN_NOT_OK_PREPEND(
+      TSTabletManager::DeleteTabletData(meta_, tablet::TABLET_DATA_TOMBSTONED, boost::none),
+      LogPrefix() + "Failed to tombstone tablet after aborting tablet copy");
+
+  UpdateStatusMessage(Substitute("Tombstoned tablet $0: Tablet copy aborted", tablet_id_));
+  return Status::OK();
+}
+
 // Decode the remote error into a human-readable Status object.
 Status TabletCopyClient::ExtractRemoteError(const rpc::ErrorStatusPB& remote_error) {
   if (PREDICT_TRUE(remote_error.HasExtension(TabletCopyErrorPB::tablet_copy_error_ext))) {
@@ -302,7 +341,7 @@ Status TabletCopyClient::ExtractRemoteError(const rpc::ErrorStatusPB& remote_err
 
 // Enhance a RemoteError Status message with additional details from the remote.
 Status TabletCopyClient::UnwindRemoteError(const Status& status,
-                                                const rpc::RpcController& controller) {
+                                           const rpc::RpcController& controller) {
   if (!status.IsRemoteError()) {
     return status;
   }
@@ -312,18 +351,17 @@ Status TabletCopyClient::UnwindRemoteError(const Status& status,
 
 void TabletCopyClient::UpdateStatusMessage(const string& message) {
   if (status_listener_ != nullptr) {
-    status_listener_->StatusMessage("TabletCopy: " + message);
+    status_listener_->StatusMessage(Substitute("Tablet Copy: $0", message));
   }
 }
 
 Status TabletCopyClient::EndRemoteSession() {
-  if (!started_) {
+  if (state_ == kInitialized) {
     return Status::OK();
   }
 
   rpc::RpcController controller;
-  controller.set_timeout(MonoDelta::FromMilliseconds(
-        FLAGS_tablet_copy_begin_session_timeout_ms));
+  controller.set_timeout(MonoDelta::FromMilliseconds(FLAGS_tablet_copy_begin_session_timeout_ms));
 
   EndTabletCopySessionRequestPB req;
   req.set_session_id(session_id_);
@@ -337,7 +375,7 @@ Status TabletCopyClient::EndRemoteSession() {
 }
 
 Status TabletCopyClient::DownloadWALs() {
-  CHECK(started_);
+  CHECK_EQ(kStarted, state_);
 
   // Delete and recreate WAL dir if it already exists, to ensure stray files are
   // not kept from previous copies and runs.
@@ -359,16 +397,12 @@ Status TabletCopyClient::DownloadWALs() {
     ++counter;
   }
 
-  downloaded_wal_ = true;
   return Status::OK();
 }
 
-Status TabletCopyClient::DownloadBlocks() {
-  CHECK(started_);
-
-  // Count up the total number of blocks to download.
+int TabletCopyClient::CountRemoteBlocks() const {
   int num_blocks = 0;
-  for (const RowSetDataPB& rowset : superblock_->rowsets()) {
+  for (const RowSetDataPB& rowset : remote_superblock_->rowsets()) {
     num_blocks += rowset.columns_size();
     num_blocks += rowset.redo_deltas_size();
     num_blocks += rowset.undo_deltas_size();
@@ -379,42 +413,74 @@ Status TabletCopyClient::DownloadBlocks() {
       num_blocks++;
     }
   }
+  return num_blocks;
+}
+
+Status TabletCopyClient::DownloadBlocks() {
+  CHECK_EQ(kStarted, state_);
+
+  // Count up the total number of blocks to download.
+  int num_remote_blocks = CountRemoteBlocks();
 
   // Download each block, writing the new block IDs into the new superblock
   // as each block downloads.
-  gscoped_ptr<TabletSuperBlockPB> new_sb(new TabletSuperBlockPB());
-  new_sb->CopyFrom(*superblock_);
   int block_count = 0;
-  LOG_WITH_PREFIX(INFO) << "Starting download of " << num_blocks << " data blocks...";
-  for (RowSetDataPB& rowset : *new_sb->mutable_rowsets()) {
-    for (ColumnDataPB& col : *rowset.mutable_columns()) {
-      RETURN_NOT_OK(DownloadAndRewriteBlock(col.mutable_block(),
-                                            &block_count, num_blocks));
+  LOG_WITH_PREFIX(INFO) << "Starting download of " << num_remote_blocks << " data blocks...";
+  for (const RowSetDataPB& src_rowset : remote_superblock_->rowsets()) {
+    // Create rowset.
+    RowSetDataPB* dst_rowset = superblock_->add_rowsets();
+    *dst_rowset = src_rowset;
+    // Clear the data in the rowset so that we don't end up deleting the wrong
+    // blocks (using the ids of the remote blocks) if we fail.
+    // TODO(mpercy): This is pretty fragile. Consider building a class
+    // structure on top of SuperBlockPB to abstract copying details.
+    dst_rowset->clear_columns();
+    dst_rowset->clear_redo_deltas();
+    dst_rowset->clear_undo_deltas();
+    dst_rowset->clear_bloom_block();
+    dst_rowset->clear_adhoc_index_block();
+
+    // We can't leave superblock_ unserializable with unset required field
+    // values in child elements, so we must download and rewrite each block
+    // before referencing it in the rowset.
+    for (const ColumnDataPB& src_col : src_rowset.columns()) {
+      BlockIdPB new_block_id;
+      RETURN_NOT_OK(DownloadAndRewriteBlock(src_col.block(), num_remote_blocks,
+                                            &block_count, &new_block_id));
+      ColumnDataPB* dst_col = dst_rowset->add_columns();
+      *dst_col = src_col;
+      *dst_col->mutable_block() = new_block_id;
     }
-    for (DeltaDataPB& redo : *rowset.mutable_redo_deltas()) {
-      RETURN_NOT_OK(DownloadAndRewriteBlock(redo.mutable_block(),
-                                            &block_count, num_blocks));
+    for (const DeltaDataPB& src_redo : src_rowset.redo_deltas()) {
+      BlockIdPB new_block_id;
+      RETURN_NOT_OK(DownloadAndRewriteBlock(src_redo.block(), num_remote_blocks,
+                                            &block_count, &new_block_id));
+      DeltaDataPB* dst_redo = dst_rowset->add_redo_deltas();
+      *dst_redo = src_redo;
+      *dst_redo->mutable_block() = new_block_id;
     }
-    for (DeltaDataPB& undo : *rowset.mutable_undo_deltas()) {
-      RETURN_NOT_OK(DownloadAndRewriteBlock(undo.mutable_block(),
-                                            &block_count, num_blocks));
+    for (const DeltaDataPB& src_undo : src_rowset.undo_deltas()) {
+      BlockIdPB new_block_id;
+      RETURN_NOT_OK(DownloadAndRewriteBlock(src_undo.block(), num_remote_blocks,
+                                            &block_count, &new_block_id));
+      DeltaDataPB* dst_undo = dst_rowset->add_undo_deltas();
+      *dst_undo = src_undo;
+      *dst_undo->mutable_block() = new_block_id;
     }
-    if (rowset.has_bloom_block()) {
-      RETURN_NOT_OK(DownloadAndRewriteBlock(rowset.mutable_bloom_block(),
-                                            &block_count, num_blocks));
+    if (src_rowset.has_bloom_block()) {
+      BlockIdPB new_block_id;
+      RETURN_NOT_OK(DownloadAndRewriteBlock(src_rowset.bloom_block(), num_remote_blocks,
+                                            &block_count, &new_block_id));
+      *dst_rowset->mutable_bloom_block() = new_block_id;
     }
-    if (rowset.has_adhoc_index_block()) {
-      RETURN_NOT_OK(DownloadAndRewriteBlock(rowset.mutable_adhoc_index_block(),
-                                            &block_count, num_blocks));
+    if (src_rowset.has_adhoc_index_block()) {
+      BlockIdPB new_block_id;
+      RETURN_NOT_OK(DownloadAndRewriteBlock(src_rowset.adhoc_index_block(), num_remote_blocks,
+                                            &block_count, &new_block_id));
+      *dst_rowset->mutable_adhoc_index_block() = new_block_id;
     }
   }
 
-  // The orphaned physical block ids at the remote have no meaning to us.
-  new_sb->clear_orphaned_blocks();
-  new_superblock_.swap(new_sb);
-
-  downloaded_blocks_ = true;
-
   return Status::OK();
 }
 
@@ -462,23 +528,25 @@ Status TabletCopyClient::WriteConsensusMetadata() {
   return Status::OK();
 }
 
-Status TabletCopyClient::DownloadAndRewriteBlock(BlockIdPB* block_id,
-                                                      int* block_count, int num_blocks) {
-  BlockId old_block_id(BlockId::FromPB(*block_id));
+Status TabletCopyClient::DownloadAndRewriteBlock(const BlockIdPB& src_block_id,
+                                                 int num_blocks,
+                                                 int* block_count,
+                                                 BlockIdPB* dest_block_id) {
+  BlockId old_block_id(BlockId::FromPB(src_block_id));
   UpdateStatusMessage(Substitute("Downloading block $0 ($1/$2)",
-                                 old_block_id.ToString(), *block_count,
-                                 num_blocks));
+                                 old_block_id.ToString(),
+                                 *block_count + 1, num_blocks));
   BlockId new_block_id;
   RETURN_NOT_OK_PREPEND(DownloadBlock(old_block_id, &new_block_id),
       "Unable to download block with id " + old_block_id.ToString());
 
-  new_block_id.CopyToPB(block_id);
+  new_block_id.CopyToPB(dest_block_id);
   (*block_count)++;
   return Status::OK();
 }
 
 Status TabletCopyClient::DownloadBlock(const BlockId& old_block_id,
-                                            BlockId* new_block_id) {
+                                       BlockId* new_block_id) {
   VLOG_WITH_PREFIX(1) << "Downloading block with block_id " << old_block_id.ToString();
 
   unique_ptr<WritableBlock> block;
@@ -499,7 +567,7 @@ Status TabletCopyClient::DownloadBlock(const BlockId& old_block_id,
 
 template<class Appendable>
 Status TabletCopyClient::DownloadFile(const DataIdPB& data_id,
-                                           Appendable* appendable) {
+                                      Appendable* appendable) {
   uint64_t offset = 0;
   rpc::RpcController controller;
   controller.set_timeout(MonoDelta::FromMilliseconds(session_idle_timeout_millis_));

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/tablet_copy_client.h
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_client.h b/src/kudu/tserver/tablet_copy_client.h
index de2a730..3545915 100644
--- a/src/kudu/tserver/tablet_copy_client.h
+++ b/src/kudu/tserver/tablet_copy_client.h
@@ -23,14 +23,13 @@
 
 #include <gtest/gtest_prod.h>
 
-#include "kudu/gutil/gscoped_ptr.h"
+#include "kudu/fs/block_id.h"
 #include "kudu/gutil/macros.h"
 #include "kudu/gutil/ref_counted.h"
 #include "kudu/util/status.h"
 
 namespace kudu {
 
-class BlockId;
 class BlockIdPB;
 class FsManager;
 class HostPort;
@@ -95,22 +94,35 @@ class TabletCopyClient {
   // otherwise the TabletMetadata object resulting from the initial remote
   // bootstrap response is returned.
   Status Start(const HostPort& copy_source_addr,
-               scoped_refptr<tablet::TabletMetadata>* metadata);
+               scoped_refptr<tablet::TabletMetadata>* meta);
 
   // Runs a "full" tablet copy, copying the physical layout of a tablet
   // from the leader of the specified consensus configuration.
   Status FetchAll(tablet::TabletStatusListener* status_listener);
 
   // After downloading all files successfully, write out the completed
-  // replacement superblock.
+  // replacement superblock. Must be called after Start() and FetchAll().
+  // Must not be called after Abort().
   Status Finish();
 
+  // Abort an in-progress transfer and immediately delete the data blocks and
+  // WALs downloaded so far. Does nothing if called after Finish().
+  Status Abort();
+
  private:
+  FRIEND_TEST(TabletCopyClientTest, TestNoBlocksAtStart);
   FRIEND_TEST(TabletCopyClientTest, TestBeginEndSession);
   FRIEND_TEST(TabletCopyClientTest, TestDownloadBlock);
   FRIEND_TEST(TabletCopyClientTest, TestVerifyData);
   FRIEND_TEST(TabletCopyClientTest, TestDownloadWalSegment);
   FRIEND_TEST(TabletCopyClientTest, TestDownloadAllBlocks);
+  FRIEND_TEST(TabletCopyClientAbortTest, TestAbort);
+
+  enum State {
+    kInitialized,
+    kStarted,
+    kFinished,
+  };
 
   // Extract the embedded Status message from the given ErrorStatusPB.
   // The given ErrorStatusPB must extend TabletCopyErrorPB.
@@ -137,19 +149,26 @@ class TabletCopyClient {
   // downloaded as part of initiating the tablet copy session.
   Status WriteConsensusMetadata();
 
+  // Count the number of blocks on the remote (from 'remote_superblock_').
+  int CountRemoteBlocks() const;
+
   // Download all blocks belonging to a tablet sequentially.
   //
-  // Blocks are given new IDs upon creation. On success, 'new_superblock_'
-  // is populated to reflect the new block IDs and should be used in lieu
-  // of 'superblock_' henceforth.
+  // Blocks are given new IDs upon creation. On success, 'superblock_'
+  // is populated to reflect the new block IDs.
   Status DownloadBlocks();
 
-  // Download the block specified by 'block_id'.
+  // Download the remote block specified by 'src_block_id'. 'num_blocks' should
+  // be given as the total number of blocks there are to download (for logging
+  // purposes).
   //
   // On success:
-  // - 'block_id' is set to the new ID of the downloaded block.
-  // - 'block_count' is incremented.
-  Status DownloadAndRewriteBlock(BlockIdPB* block_id, int* block_count, int num_blocks);
+  // - 'dest_block_id' is set to the new ID of the downloaded block.
+  // - 'block_count' is incremented by 1.
+  Status DownloadAndRewriteBlock(const BlockIdPB& src_block_id,
+                                 int num_blocks,
+                                 int* block_count,
+                                 BlockIdPB* dest_block_id);
 
   // Download a single block.
   // Data block is opened with options so that it will fsync() on close.
@@ -177,10 +196,8 @@ class TabletCopyClient {
   FsManager* const fs_manager_;
   const std::shared_ptr<rpc::Messenger> messenger_;
 
-  // State flags that enforce the progress of tablet copy.
-  bool started_;            // Session started.
-  bool downloaded_wal_;     // WAL segments downloaded.
-  bool downloaded_blocks_;  // Data blocks downloaded.
+  // State of the progress of the tablet copy operation.
+  State state_;
 
   // Session-specific data items.
   bool replace_tombstoned_tablet_;
@@ -196,9 +213,9 @@ class TabletCopyClient {
   std::shared_ptr<TabletCopyServiceProxy> proxy_;
   std::string session_id_;
   uint64_t session_idle_timeout_millis_;
-  gscoped_ptr<tablet::TabletSuperBlockPB> superblock_;
-  gscoped_ptr<tablet::TabletSuperBlockPB> new_superblock_;
-  gscoped_ptr<consensus::ConsensusStatePB> remote_committed_cstate_;
+  std::unique_ptr<tablet::TabletSuperBlockPB> remote_superblock_;
+  std::unique_ptr<tablet::TabletSuperBlockPB> superblock_;
+  std::unique_ptr<consensus::ConsensusStatePB> remote_committed_cstate_;
   std::vector<uint64_t> wal_seqnos_;
   int64_t start_time_micros_;
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index f8a0f4f..e5df72c 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -1087,11 +1087,10 @@ void TabletServiceImpl::ScannerKeepAlive(const ScannerKeepAliveRequestPB *req,
   DCHECK(req->has_scanner_id());
   SharedScanner scanner;
   if (!server_->scanner_manager()->LookupScanner(req->scanner_id(), &scanner)) {
-      resp->mutable_error()->set_code(TabletServerErrorPB::SCANNER_EXPIRED);
-      StatusToPB(Status::NotFound("Scanner not found"),
-                 resp->mutable_error()->mutable_status());
-      context->RespondSuccess();
-      return;
+    resp->mutable_error()->set_code(TabletServerErrorPB::SCANNER_EXPIRED);
+    StatusToPB(Status::NotFound("Scanner not found"), resp->mutable_error()->mutable_status());
+    context->RespondSuccess();
+    return;
   }
   scanner->UpdateAccessTime();
   context->RespondSuccess();

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/ts_tablet_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/ts_tablet_manager.cc b/src/kudu/tserver/ts_tablet_manager.cc
index 8214655..9bfa941 100644
--- a/src/kudu/tserver/ts_tablet_manager.cc
+++ b/src/kudu/tserver/ts_tablet_manager.cc
@@ -317,16 +317,6 @@ Status TSTabletManager::CreateNewTablet(const string& table_id,
   return Status::OK();
 }
 
-// If 'expr' fails, log a message, tombstone the given tablet, and return.
-#define TOMBSTONE_NOT_OK(expr, peer, msg) \
-  do { \
-    const Status& _s = (expr); \
-    if (PREDICT_FALSE(!_s.ok())) { \
-      LogAndTombstone((peer), (msg), _s); \
-      return; \
-    } \
-  } while (0)
-
 Status TSTabletManager::CheckLeaderTermNotLower(const string& tablet_id,
                                                 int64_t leader_term,
                                                 int64_t last_logged_term) {
@@ -424,8 +414,9 @@ void TSTabletManager::RunTabletCopy(
 
   TabletServerErrorPB::Code error_code = TabletServerErrorPB::UNKNOWN_ERROR;
 
-  const string& tablet_id = req->tablet_id();
-  const string& copy_source_uuid = req->copy_peer_uuid();
+  // Copy these strings so they stay valid even after responding to the request.
+  string tablet_id = req->tablet_id(); // NOLINT(*)
+  string copy_source_uuid = req->copy_peer_uuid(); // NOLINT(*)
   HostPort copy_source_addr;
   CALLBACK_RETURN_NOT_OK(HostPortFromPB(req->copy_peer_addr(), &copy_source_addr));
   int64_t leader_term = req->caller_term();
@@ -454,7 +445,7 @@ void TSTabletManager::RunTabletCopy(
     switch (data_state) {
       case TABLET_DATA_COPYING:
         // This should not be possible due to the transition_in_progress_ "lock".
-        LOG(FATAL) << LogPrefix(tablet_id) << " Tablet Copy: "
+        LOG(FATAL) << LogPrefix(tablet_id) << "Tablet Copy: "
                    << "Found tablet in TABLET_DATA_COPYING state during StartTabletCopy()";
       case TABLET_DATA_TOMBSTONED: {
         int64_t last_logged_term = meta->tombstone_last_logged_opid().term();
@@ -474,10 +465,10 @@ void TSTabletManager::RunTabletCopy(
 
         // Tombstone the tablet and store the last-logged OpId.
         old_tablet_peer->Shutdown();
-        // TODO: Because we begin shutdown of the tablet after we check our
+        // TODO(mpercy): Because we begin shutdown of the tablet after we check our
         // last-logged term against the leader's term, there may be operations
-        // in flight and it may be possible for the same check in the remote
-        // bootstrap client Start() method to fail. This will leave the replica in
+        // in flight and it may be possible for the same check in the tablet
+        // copy client Start() method to fail. This will leave the replica in
         // a tombstoned state, and then the leader with the latest log entries
         // will simply tablet copy this replica again. We could try to
         // check again after calling Shutdown(), and if the check fails, try to
@@ -531,15 +522,22 @@ void TSTabletManager::RunTabletCopy(
 
   // Download all of the remote files.
   Status s = tc_client.FetchAll(implicit_cast<TabletStatusListener*>(tablet_peer.get()));
-  TOMBSTONE_NOT_OK(s, tablet_peer,
-                   Substitute("Tablet Copy: Unable to fetch data from remote peer $0",
-                              kSrcPeerInfo));
+  if (!s.ok()) {
+    LOG(WARNING) << LogPrefix(tablet_id) << "Tablet Copy: Unable to fetch data from remote peer "
+                                         << kSrcPeerInfo << ": " << s.ToString();
+    return;
+  }
 
   MAYBE_FAULT(FLAGS_fault_crash_after_tc_files_fetched);
 
   // Write out the last files to make the new replica visible and update the
   // TabletDataState in the superblock to TABLET_DATA_READY.
-  TOMBSTONE_NOT_OK(tc_client.Finish(), tablet_peer, "Tablet Copy: Failure calling Finish()");
+  s = tc_client.Finish();
+  if (!s.ok()) {
+    LOG(WARNING) << LogPrefix(tablet_id) << "Tablet Copy: Failure calling Finish(): "
+                                         << s.ToString();
+    return;
+  }
 
   // We don't tombstone the tablet if opening the tablet fails, because on next
   // startup it's still in a valid, fully-copied state.
@@ -698,7 +696,7 @@ Status TSTabletManager::StartTabletStateTransitionUnlocked(
 
 Status TSTabletManager::OpenTabletMeta(const string& tablet_id,
                                        scoped_refptr<TabletMetadata>* metadata) {
-  LOG(INFO) << "Loading metadata for tablet " << tablet_id;
+  LOG(INFO) << LogPrefix(tablet_id) << "Loading tablet metadata";
   TRACE("Loading metadata...");
   scoped_refptr<TabletMetadata> meta;
   RETURN_NOT_OK_PREPEND(TabletMetadata::Load(fs_manager_, tablet_id, &meta),
@@ -860,7 +858,9 @@ void TSTabletManager::RegisterTablet(const std::string& tablet_id,
     LOG(FATAL) << "Unable to register tablet peer " << tablet_id << ": already registered!";
   }
 
-  LOG(INFO) << "Registered tablet " << tablet_id;
+  TabletDataState data_state = tablet_peer->tablet_metadata()->tablet_data_state();
+  LOG(INFO) << LogPrefix(tablet_id) << Substitute("Registered tablet (data state: $0)",
+                                                  TabletDataState_Name(data_state));
 }
 
 bool TSTabletManager::LookupTablet(const string& tablet_id,
@@ -1021,7 +1021,8 @@ Status TSTabletManager::DeleteTabletData(const scoped_refptr<TabletMetadata>& me
             << "Deleting tablet data with delete state "
             << TabletDataState_Name(data_state);
   CHECK(data_state == TABLET_DATA_DELETED ||
-        data_state == TABLET_DATA_TOMBSTONED)
+        data_state == TABLET_DATA_TOMBSTONED ||
+        data_state == TABLET_DATA_COPYING)
       << "Unexpected data_state to delete tablet " << meta->tablet_id() << ": "
       << TabletDataState_Name(data_state) << " (" << data_state << ")";
 
@@ -1037,33 +1038,19 @@ Status TSTabletManager::DeleteTabletData(const scoped_refptr<TabletMetadata>& me
   MAYBE_FAULT(FLAGS_fault_crash_after_wal_deleted);
 
   // We do not delete the superblock or the consensus metadata when tombstoning
-  // a tablet.
-  if (data_state == TABLET_DATA_TOMBSTONED) {
+  // a tablet or marking it as entering the tablet copy process.
+  if (data_state == TABLET_DATA_COPYING ||
+      data_state == TABLET_DATA_TOMBSTONED) {
     return Status::OK();
   }
 
   // Only TABLET_DATA_DELETED tablets get this far.
+  DCHECK_EQ(TABLET_DATA_DELETED, data_state);
   RETURN_NOT_OK(ConsensusMetadata::DeleteOnDiskData(meta->fs_manager(), meta->tablet_id()));
   MAYBE_FAULT(FLAGS_fault_crash_after_cmeta_deleted);
   return meta->DeleteSuperBlock();
 }
 
-void TSTabletManager::LogAndTombstone(const scoped_refptr<TabletPeer>& peer,
-                                      const std::string& msg,
-                                      const Status& s) {
-  const string& tablet_id = peer->tablet_id();
-  LOG(WARNING) << LogPrefix(tablet_id) << msg << ": " << s.ToString();
-
-  Status delete_status = DeleteTabletData(
-      peer->tablet_metadata(), TABLET_DATA_TOMBSTONED, boost::optional<OpId>());
-  if (PREDICT_FALSE(!delete_status.ok())) {
-    // This failure should only either indicate a bug or an IO error.
-    LOG(FATAL) << LogPrefix(tablet_id) << "Failed to tombstone tablet after tablet copy: "
-               << delete_status.ToString();
-  }
-  peer->StatusMessage(Substitute("Tombstoned tablet: $0 ($1)", msg, s.ToString()));
-}
-
 TransitionInProgressDeleter::TransitionInProgressDeleter(
     TransitionInProgressMap* map, rw_spinlock* lock, string entry)
     : in_progress_(map), lock_(lock), entry_(std::move(entry)) {}

http://git-wip-us.apache.org/repos/asf/kudu/blob/5ae27d73/src/kudu/tserver/ts_tablet_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/ts_tablet_manager.h b/src/kudu/tserver/ts_tablet_manager.h
index 43f5396..6f437be 100644
--- a/src/kudu/tserver/ts_tablet_manager.h
+++ b/src/kudu/tserver/ts_tablet_manager.h
@@ -270,13 +270,6 @@ class TSTabletManager : public tserver::TabletPeerLookupIf {
                                  int64_t leader_term,
                                  int64_t last_logged_term);
 
-  // Print a log message using the given info and tombstone the specified
-  // tablet. If tombstoning the tablet fails, a FATAL error is logged, resulting
-  // in a crash.
-  void LogAndTombstone(const scoped_refptr<tablet::TabletPeer>& peer,
-                       const std::string& msg,
-                       const Status& s);
-
   TSTabletManagerStatePB state() const {
     shared_lock<rw_spinlock> l(lock_);
     return state_;


Mime
View raw message