kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject [5/5] kudu git commit: ConsensusMetadata::Create() should not overwrite an existing file
Date Wed, 28 Jun 2017 18:44:10 GMT
ConsensusMetadata::Create() should not overwrite an existing file

We want to be able to control whether clobbering files is allowed. For
example, when there is an existing ConsensusMetadata file on disk, it is
dangerous for Create() to overwrite that file. The safe procedure to
make changes in that situation is to first Load() the file, make
changes, then Flush().

* Stop allowing ConsensusMetadata::Create() to overwrite files.
* Add unit test ConsensusMetadataTest.TestCreateNoOverwrite for this.
* Fix test BootstrapTest.TestOrphanCommit that violated this rule.

Change-Id: I0bfef1ae86a7d2c162095a76bb184a9c18dc69a7
Reviewed-on: http://gerrit.cloudera.org:8080/7190
Tested-by: Mike Percy <mpercy@apache.org>
Reviewed-by: Mike Percy <mpercy@apache.org>


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

Branch: refs/heads/master
Commit: e9622028cb9b54d3b93af2a2a35db64e02052749
Parents: cd8e7af
Author: Mike Percy <mpercy@apache.org>
Authored: Fri Jun 9 16:45:37 2017 -0700
Committer: Mike Percy <mpercy@apache.org>
Committed: Wed Jun 28 18:42:50 2017 +0000

----------------------------------------------------------------------
 src/kudu/consensus/consensus_meta-test.cc | 13 +++++++++
 src/kudu/consensus/consensus_meta.cc      |  6 ++--
 src/kudu/consensus/consensus_meta.h       |  9 +++++-
 src/kudu/tablet/tablet_bootstrap-test.cc  | 38 ++++++++++++++++----------
 4 files changed, 48 insertions(+), 18 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/e9622028/src/kudu/consensus/consensus_meta-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_meta-test.cc b/src/kudu/consensus/consensus_meta-test.cc
index 33f9e0c..2faee64 100644
--- a/src/kudu/consensus/consensus_meta-test.cc
+++ b/src/kudu/consensus/consensus_meta-test.cc
@@ -99,6 +99,19 @@ TEST_F(ConsensusMetadataTest, TestCreateLoad) {
   ASSERT_VALUES_EQUAL(*cmeta, kInvalidOpIdIndex, fs_manager_.uuid(), kInitialTerm);
 }
 
+// Ensure that Create() will not overwrite an existing file.
+TEST_F(ConsensusMetadataTest, TestCreateNoOverwrite) {
+  unique_ptr<ConsensusMetadata> cmeta;
+  // Create the consensus metadata file.
+  ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, fs_manager_.uuid(),
+                                      config_, kInitialTerm, &cmeta));
+  // Try to create it again.
+  Status s = ConsensusMetadata::Create(&fs_manager_, kTabletId, fs_manager_.uuid(),
+                                       config_, kInitialTerm, &cmeta);
+  ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
+  ASSERT_STR_MATCHES(s.ToString(), "Unable to write consensus meta file.*already exists");
+}
+
 // Ensure that we get an error when loading a file that doesn't exist.
 TEST_F(ConsensusMetadataTest, TestFailedLoad) {
   unique_ptr<ConsensusMetadata> cmeta;

http://git-wip-us.apache.org/repos/asf/kudu/blob/e9622028/src/kudu/consensus/consensus_meta.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_meta.cc b/src/kudu/consensus/consensus_meta.cc
index ec46755..c2643d4 100644
--- a/src/kudu/consensus/consensus_meta.cc
+++ b/src/kudu/consensus/consensus_meta.cc
@@ -51,7 +51,7 @@ Status ConsensusMetadata::Create(FsManager* fs_manager,
   unique_ptr<ConsensusMetadata> cmeta(new ConsensusMetadata(fs_manager, tablet_id,
peer_uuid));
   cmeta->set_committed_config(config);
   cmeta->set_current_term(current_term);
-  RETURN_NOT_OK(cmeta->Flush());
+  RETURN_NOT_OK(cmeta->Flush(NO_OVERWRITE)); // Create() should not clobber.
   cmeta_out->swap(cmeta);
   return Status::OK();
 }
@@ -184,7 +184,7 @@ void ConsensusMetadata::MergeCommittedConsensusStatePB(const ConsensusStatePB&
c
   clear_pending_config();
 }
 
-Status ConsensusMetadata::Flush() {
+Status ConsensusMetadata::Flush(FlushMode mode) {
   MAYBE_FAULT(FLAGS_fault_crash_before_cmeta_flush);
   SCOPED_LOG_SLOW_EXECUTION_PREFIX(WARNING, 500, LogPrefix(), "flushing consensus metadata");
 
@@ -208,7 +208,7 @@ Status ConsensusMetadata::Flush() {
   string meta_file_path = fs_manager_->GetConsensusMetadataPath(tablet_id_);
   RETURN_NOT_OK_PREPEND(pb_util::WritePBContainerToPath(
       fs_manager_->env(), meta_file_path, pb_,
-      pb_util::OVERWRITE,
+      mode == OVERWRITE ? pb_util::OVERWRITE : pb_util::NO_OVERWRITE,
       // We use FLAGS_log_force_fsync_all here because the consensus metadata is
       // essentially an extension of the primary durability mechanism of the
       // consensus subsystem: the WAL. Using the same flag ensures that the WAL

http://git-wip-us.apache.org/repos/asf/kudu/blob/e9622028/src/kudu/consensus/consensus_meta.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_meta.h b/src/kudu/consensus/consensus_meta.h
index f4e3d7a..a903675 100644
--- a/src/kudu/consensus/consensus_meta.h
+++ b/src/kudu/consensus/consensus_meta.h
@@ -56,6 +56,13 @@ namespace consensus {
 // This class is not thread-safe and requires external synchronization.
 class ConsensusMetadata {
  public:
+
+  // Specify whether we are allowed to overwrite an existing file when flushing.
+  enum FlushMode {
+    OVERWRITE,
+    NO_OVERWRITE
+  };
+
   // Create a ConsensusMetadata object with provided initial state.
   // Encoded PB is flushed to disk before returning.
   static Status Create(FsManager* fs_manager,
@@ -136,7 +143,7 @@ class ConsensusMetadata {
   void MergeCommittedConsensusStatePB(const ConsensusStatePB& cstate);
 
   // Persist current state of the protobuf to disk.
-  Status Flush();
+  Status Flush(FlushMode mode = OVERWRITE);
 
   int flush_count_for_tests() const {
     return flush_count_for_tests_;

http://git-wip-us.apache.org/repos/asf/kudu/blob/e9622028/src/kudu/tablet/tablet_bootstrap-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_bootstrap-test.cc b/src/kudu/tablet/tablet_bootstrap-test.cc
index 4a3f735..ec89131 100644
--- a/src/kudu/tablet/tablet_bootstrap-test.cc
+++ b/src/kudu/tablet/tablet_bootstrap-test.cc
@@ -85,6 +85,21 @@ class BootstrapTest : public LogTestBase {
     return (*meta)->Flush();
   }
 
+  Status CreateConsensusMetadata(const scoped_refptr<TabletMetadata>& meta) {
+    consensus::RaftConfigPB config;
+    config.set_opid_index(consensus::kInvalidOpIdIndex);
+    consensus::RaftPeerPB* peer = config.add_peers();
+    peer->set_permanent_uuid(meta->fs_manager()->uuid());
+    peer->set_member_type(consensus::RaftPeerPB::VOTER);
+
+    unique_ptr<ConsensusMetadata> cmeta;
+    RETURN_NOT_OK_PREPEND(ConsensusMetadata::Create(meta->fs_manager(), meta->tablet_id(),
+                                                    meta->fs_manager()->uuid(),
+                                                    config, kMinimumTerm, &cmeta),
+                          "Unable to create consensus metadata");
+    return Status::OK();
+  }
+
   Status PersistTestTabletMetadataState(TabletDataState state) {
     scoped_refptr<TabletMetadata> meta;
     RETURN_NOT_OK(LoadTestTabletMetadata(-1, -1, &meta));
@@ -121,17 +136,7 @@ class BootstrapTest : public LogTestBase {
     RETURN_NOT_OK_PREPEND(LoadTestTabletMetadata(mrs_id, delta_id, &meta),
                           "Unable to load test tablet metadata");
 
-    consensus::RaftConfigPB config;
-    config.set_opid_index(consensus::kInvalidOpIdIndex);
-    consensus::RaftPeerPB* peer = config.add_peers();
-    peer->set_permanent_uuid(meta->fs_manager()->uuid());
-    peer->set_member_type(consensus::RaftPeerPB::VOTER);
-
-    unique_ptr<ConsensusMetadata> cmeta;
-    RETURN_NOT_OK_PREPEND(ConsensusMetadata::Create(meta->fs_manager(), meta->tablet_id(),
-                                                    meta->fs_manager()->uuid(),
-                                                    config, kMinimumTerm, &cmeta),
-                          "Unable to create consensus metadata");
+    RETURN_NOT_OK(CreateConsensusMetadata(meta));
 
     RETURN_NOT_OK_PREPEND(RunBootstrapOnTestTablet(meta, tablet, boot_info),
                           "Unable to bootstrap test tablet");
@@ -244,19 +249,23 @@ TEST_F(BootstrapTest, TestOrphanCommit) {
   // Step 2) Write the corresponding COMMIT in the second segment.
   AppendCommit(opid);
 
+  scoped_refptr<TabletMetadata> meta;
+  ASSERT_OK(LoadTestTabletMetadata(/*mrs_id=*/ -1, /*delta_id=*/ -1, &meta));
+  ASSERT_OK(CreateConsensusMetadata(meta));
+
   {
     shared_ptr<Tablet> tablet;
     ConsensusBootstrapInfo boot_info;
 
     // Step 3) Apply the operations in the log to the tablet and flush
     // the tablet to disk.
-    ASSERT_OK(BootstrapTestTablet(-1, -1, &tablet, &boot_info));
+    ASSERT_OK(RunBootstrapOnTestTablet(meta, &tablet, &boot_info));
     ASSERT_OK(tablet->Flush());
 
     // Create a new log segment.
     ASSERT_OK(RollLog());
 
-    // Step 4) Create an orphanned commit by first adding a commit to
+    // Step 4) Create an orphaned commit by first adding a commit to
     // the newly rolled logfile, and then by removing the previous
     // commits.
     AppendCommit(opid);
@@ -274,7 +283,8 @@ TEST_F(BootstrapTest, TestOrphanCommit) {
 
     // Note: when GLOG_v=1, the test logs should include 'Ignoring
     // orphan commit: op_type: WRITE_OP...' line.
-    ASSERT_OK(BootstrapTestTablet(2, 1, &tablet, &boot_info));
+    ASSERT_OK(LoadTestTabletMetadata(/*mrs_id=*/ 2, /*delta_id=*/ 1, &meta));
+    ASSERT_OK(RunBootstrapOnTestTablet(meta, &tablet, &boot_info));
 
     // Confirm that the legitimate data (from Step 3) is still there.
     vector<string> results;


Mime
View raw message