Skip to content

Commit

Permalink
Replace gscoped_ptr with unique_ptr for ConsensusMetadata
Browse files Browse the repository at this point in the history
Change-Id: Iffc81148d497f1da95ccd25cbfc487c3bef71a6c
Reviewed-on: http://gerrit.cloudera.org:8080/3967
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <[email protected]>
  • Loading branch information
adembo committed Aug 15, 2016
1 parent b7e4f12 commit 28a9902
Show file tree
Hide file tree
Showing 19 changed files with 82 additions and 56 deletions.
23 changes: 12 additions & 11 deletions src/kudu/consensus/consensus_meta-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.
#include "kudu/consensus/consensus_meta.h"

#include <memory>
#include <vector>

#include <gtest/gtest.h>
Expand All @@ -25,7 +26,6 @@
#include "kudu/consensus/opid_util.h"
#include "kudu/consensus/quorum_util.h"
#include "kudu/fs/fs_manager.h"
#include "kudu/gutil/gscoped_ptr.h"
#include "kudu/util/net/net_util.h"
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
Expand All @@ -37,6 +37,7 @@ namespace kudu {
namespace consensus {

using std::string;
using std::unique_ptr;
using std::vector;

const char* kTabletId = "test-consensus-metadata";
Expand Down Expand Up @@ -86,20 +87,20 @@ void ConsensusMetadataTest::AssertValuesEqual(const ConsensusMetadata& cmeta,
TEST_F(ConsensusMetadataTest, TestCreateLoad) {
// Create the file.
{
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, fs_manager_.uuid(),
config_, kInitialTerm, &cmeta));
}

// Load the file.
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Load(&fs_manager_, kTabletId, fs_manager_.uuid(), &cmeta));
ASSERT_VALUES_EQUAL(*cmeta, kInvalidOpIdIndex, fs_manager_.uuid(), kInitialTerm);
}

// Ensure that we get an error when loading a file that doesn't exist.
TEST_F(ConsensusMetadataTest, TestFailedLoad) {
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
Status s = ConsensusMetadata::Load(&fs_manager_, kTabletId, fs_manager_.uuid(), &cmeta);
ASSERT_TRUE(s.IsNotFound()) << "Unexpected status: " << s.ToString();
LOG(INFO) << "Expected failure: " << s.ToString();
Expand All @@ -108,7 +109,7 @@ TEST_F(ConsensusMetadataTest, TestFailedLoad) {
// Check that changes are not written to disk until Flush() is called.
TEST_F(ConsensusMetadataTest, TestFlush) {
const int64_t kNewTerm = 4;
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, fs_manager_.uuid(),
config_, kInitialTerm, &cmeta));
cmeta->set_current_term(kNewTerm);
Expand All @@ -117,15 +118,15 @@ TEST_F(ConsensusMetadataTest, TestFlush) {
// objects in flight that point to the same file, but for a test this is fine
// since it's read-only.
{
gscoped_ptr<ConsensusMetadata> cmeta_read;
unique_ptr<ConsensusMetadata> cmeta_read;
ASSERT_OK(ConsensusMetadata::Load(&fs_manager_, kTabletId, fs_manager_.uuid(), &cmeta_read));
ASSERT_VALUES_EQUAL(*cmeta_read, kInvalidOpIdIndex, fs_manager_.uuid(), kInitialTerm);
}

ASSERT_OK(cmeta->Flush());

{
gscoped_ptr<ConsensusMetadata> cmeta_read;
unique_ptr<ConsensusMetadata> cmeta_read;
ASSERT_OK(ConsensusMetadata::Load(&fs_manager_, kTabletId, fs_manager_.uuid(), &cmeta_read));
ASSERT_VALUES_EQUAL(*cmeta_read, kInvalidOpIdIndex, fs_manager_.uuid(), kNewTerm);
}
Expand All @@ -150,7 +151,7 @@ TEST_F(ConsensusMetadataTest, TestActiveRole) {
RaftConfigPB config1 = BuildConfig(uuids); // We aren't a member of this config...
config1.set_opid_index(1);

gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, peer_uuid,
config1, kInitialTerm, &cmeta));

Expand Down Expand Up @@ -195,7 +196,7 @@ TEST_F(ConsensusMetadataTest, TestToConsensusStatePB) {

RaftConfigPB committed_config = BuildConfig(uuids); // We aren't a member of this config...
committed_config.set_opid_index(1);
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, peer_uuid,
committed_config, kInitialTerm, &cmeta));

Expand Down Expand Up @@ -227,7 +228,7 @@ TEST_F(ConsensusMetadataTest, TestToConsensusStatePB) {
}

// Helper for TestMergeCommittedConsensusStatePB.
static void AssertConsensusMergeExpected(const gscoped_ptr<ConsensusMetadata>& cmeta,
static void AssertConsensusMergeExpected(const unique_ptr<ConsensusMetadata>& cmeta,
const ConsensusStatePB& cstate,
int64_t expected_term,
const string& expected_voted_for) {
Expand All @@ -250,7 +251,7 @@ TEST_F(ConsensusMetadataTest, TestMergeCommittedConsensusStatePB) {

RaftConfigPB committed_config = BuildConfig(uuids); // We aren't a member of this config...
committed_config.set_opid_index(1);
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, "e",
committed_config, 1, &cmeta));

Expand Down
11 changes: 7 additions & 4 deletions src/kudu/consensus/consensus_meta.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
// under the License.
#include "kudu/consensus/consensus_meta.h"

#include <memory>

#include "kudu/consensus/log_util.h"
#include "kudu/consensus/metadata.pb.h"
#include "kudu/consensus/opid_util.h"
Expand All @@ -30,15 +32,16 @@ namespace kudu {
namespace consensus {

using std::string;
using std::unique_ptr;
using strings::Substitute;

Status ConsensusMetadata::Create(FsManager* fs_manager,
const string& tablet_id,
const std::string& peer_uuid,
const RaftConfigPB& config,
int64_t current_term,
gscoped_ptr<ConsensusMetadata>* cmeta_out) {
gscoped_ptr<ConsensusMetadata> cmeta(new ConsensusMetadata(fs_manager, tablet_id, peer_uuid));
unique_ptr<ConsensusMetadata>* cmeta_out) {
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());
Expand All @@ -49,8 +52,8 @@ Status ConsensusMetadata::Create(FsManager* fs_manager,
Status ConsensusMetadata::Load(FsManager* fs_manager,
const std::string& tablet_id,
const std::string& peer_uuid,
gscoped_ptr<ConsensusMetadata>* cmeta_out) {
gscoped_ptr<ConsensusMetadata> cmeta(new ConsensusMetadata(fs_manager, tablet_id, peer_uuid));
unique_ptr<ConsensusMetadata>* cmeta_out) {
unique_ptr<ConsensusMetadata> cmeta(new ConsensusMetadata(fs_manager, tablet_id, peer_uuid));
RETURN_NOT_OK(pb_util::ReadPBContainerFromPath(fs_manager->env(),
fs_manager->GetConsensusMetadataPath(tablet_id),
&cmeta->pb_));
Expand Down
6 changes: 3 additions & 3 deletions src/kudu/consensus/consensus_meta.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
#ifndef KUDU_CONSENSUS_CONSENSUS_META_H_
#define KUDU_CONSENSUS_CONSENSUS_META_H_

#include <memory>
#include <stdint.h>
#include <string>

#include "kudu/consensus/metadata.pb.h"
#include "kudu/gutil/gscoped_ptr.h"
#include "kudu/gutil/macros.h"
#include "kudu/util/status.h"

Expand Down Expand Up @@ -63,15 +63,15 @@ class ConsensusMetadata {
const std::string& peer_uuid,
const RaftConfigPB& config,
int64_t current_term,
gscoped_ptr<ConsensusMetadata>* cmeta);
std::unique_ptr<ConsensusMetadata>* cmeta);

// Load a ConsensusMetadata object from disk.
// Returns Status::NotFound if the file could not be found. May return other
// Status codes if unable to read the file.
static Status Load(FsManager* fs_manager,
const std::string& tablet_id,
const std::string& peer_uuid,
gscoped_ptr<ConsensusMetadata>* cmeta);
std::unique_ptr<ConsensusMetadata>* cmeta);

// Delete the ConsensusMetadata file associated with the given tablet from
// disk.
Expand Down
6 changes: 4 additions & 2 deletions src/kudu/consensus/raft_consensus-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <memory>

#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol-test-util.h"
Expand All @@ -39,6 +40,7 @@ METRIC_DECLARE_entity(tablet);

using std::shared_ptr;
using std::string;
using std::unique_ptr;

namespace kudu {
namespace consensus {
Expand Down Expand Up @@ -101,7 +103,7 @@ class RaftConsensusSpy : public RaftConsensus {
typedef Callback<Status(const scoped_refptr<ConsensusRound>& round)> AppendCallback;

RaftConsensusSpy(const ConsensusOptions& options,
gscoped_ptr<ConsensusMetadata> cmeta,
unique_ptr<ConsensusMetadata> cmeta,
gscoped_ptr<PeerProxyFactory> proxy_factory,
gscoped_ptr<PeerMessageQueue> queue,
gscoped_ptr<PeerManager> peer_manager,
Expand Down Expand Up @@ -205,7 +207,7 @@ class RaftConsensusTest : public KuduTest {

string peer_uuid = config_.peers(num_peers - 1).permanent_uuid();

gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
CHECK_OK(ConsensusMetadata::Create(fs_manager_.get(), kTestTablet, peer_uuid,
config_, initial_term, &cmeta));

Expand Down
16 changes: 11 additions & 5 deletions src/kudu/consensus/raft_consensus.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <boost/optional.hpp>
#include <gflags/gflags.h>
#include <iostream>
#include <memory>
#include <mutex>

#include "kudu/common/wire_protocol.h"
Expand Down Expand Up @@ -142,6 +143,7 @@ namespace kudu {
namespace consensus {

using std::shared_ptr;
using std::unique_ptr;
using strings::Substitute;
using tserver::TabletServerErrorPB;

Expand All @@ -150,7 +152,7 @@ static const char* const kTimerId = "election-timer";

scoped_refptr<RaftConsensus> RaftConsensus::Create(
const ConsensusOptions& options,
gscoped_ptr<ConsensusMetadata> cmeta,
unique_ptr<ConsensusMetadata> cmeta,
const RaftPeerPB& local_peer_pb,
const scoped_refptr<MetricEntity>& metric_entity,
const scoped_refptr<server::Clock>& clock,
Expand Down Expand Up @@ -203,13 +205,17 @@ scoped_refptr<RaftConsensus> RaftConsensus::Create(
}

RaftConsensus::RaftConsensus(
const ConsensusOptions& options, gscoped_ptr<ConsensusMetadata> cmeta,
const ConsensusOptions& options,
unique_ptr<ConsensusMetadata> cmeta,
gscoped_ptr<PeerProxyFactory> proxy_factory,
gscoped_ptr<PeerMessageQueue> queue, gscoped_ptr<PeerManager> peer_manager,
gscoped_ptr<PeerMessageQueue> queue,
gscoped_ptr<PeerManager> peer_manager,
gscoped_ptr<ThreadPool> thread_pool,
const scoped_refptr<MetricEntity>& metric_entity,
const std::string& peer_uuid, const scoped_refptr<server::Clock>& clock,
ReplicaTransactionFactory* txn_factory, const scoped_refptr<log::Log>& log,
const std::string& peer_uuid,
const scoped_refptr<server::Clock>& clock,
ReplicaTransactionFactory* txn_factory,
const scoped_refptr<log::Log>& log,
shared_ptr<MemTracker> parent_mem_tracker,
Callback<void(const std::string& reason)> mark_dirty_clbk)
: thread_pool_(std::move(thread_pool)),
Expand Down
4 changes: 2 additions & 2 deletions src/kudu/consensus/raft_consensus.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class RaftConsensus : public Consensus,

static scoped_refptr<RaftConsensus> Create(
const ConsensusOptions& options,
gscoped_ptr<ConsensusMetadata> cmeta,
std::unique_ptr<ConsensusMetadata> cmeta,
const RaftPeerPB& local_peer_pb,
const scoped_refptr<MetricEntity>& metric_entity,
const scoped_refptr<server::Clock>& clock,
Expand All @@ -76,7 +76,7 @@ class RaftConsensus : public Consensus,
const Callback<void(const std::string& reason)>& mark_dirty_clbk);

RaftConsensus(const ConsensusOptions& options,
gscoped_ptr<ConsensusMetadata> cmeta,
std::unique_ptr<ConsensusMetadata> cmeta,
gscoped_ptr<PeerProxyFactory> peer_proxy_factory,
gscoped_ptr<PeerMessageQueue> queue,
gscoped_ptr<PeerManager> peer_manager,
Expand Down
12 changes: 7 additions & 5 deletions src/kudu/consensus/raft_consensus_quorum-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.

#include <gtest/gtest.h>
#include <memory>

#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol-test-util.h"
Expand Down Expand Up @@ -53,6 +54,7 @@ METRIC_DECLARE_entity(tablet);
ASSERT_NO_FATAL_FAILURE(ReplicateSequenceOfMessages(a, b, c, d, e, f, g))

using std::shared_ptr;
using std::unique_ptr;

namespace kudu {

Expand Down Expand Up @@ -145,7 +147,7 @@ class RaftConsensusQuorumTest : public KuduTest {

string peer_uuid = Substitute("peer-$0", i);

gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
CHECK_OK(ConsensusMetadata::Create(fs_managers_[i], kTestTablet, peer_uuid, config_,
kMinimumTerm, &cmeta));

Expand Down Expand Up @@ -527,23 +529,23 @@ class RaftConsensusQuorumTest : public KuduTest {
}

// Read the ConsensusMetadata for the given peer from disk.
gscoped_ptr<ConsensusMetadata> ReadConsensusMetadataFromDisk(int peer_index) {
unique_ptr<ConsensusMetadata> ReadConsensusMetadataFromDisk(int peer_index) {
string peer_uuid = Substitute("peer-$0", peer_index);
gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
CHECK_OK(ConsensusMetadata::Load(fs_managers_[peer_index], kTestTablet, peer_uuid, &cmeta));
return std::move(cmeta);
}

// Assert that the durable term == term and that the peer that got the vote == voted_for.
void AssertDurableTermAndVote(int peer_index, int64_t term, const std::string& voted_for) {
gscoped_ptr<ConsensusMetadata> cmeta = ReadConsensusMetadataFromDisk(peer_index);
unique_ptr<ConsensusMetadata> cmeta = ReadConsensusMetadataFromDisk(peer_index);
ASSERT_EQ(term, cmeta->current_term());
ASSERT_EQ(voted_for, cmeta->voted_for());
}

// Assert that the durable term == term and that the peer has not yet voted.
void AssertDurableTermWithoutVote(int peer_index, int64_t term) {
gscoped_ptr<ConsensusMetadata> cmeta = ReadConsensusMetadataFromDisk(peer_index);
unique_ptr<ConsensusMetadata> cmeta = ReadConsensusMetadataFromDisk(peer_index);
ASSERT_EQ(term, cmeta->current_term());
ASSERT_FALSE(cmeta->has_voted_for());
}
Expand Down
4 changes: 3 additions & 1 deletion src/kudu/consensus/raft_consensus_state-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "kudu/consensus/raft_consensus_state.h"

#include <gtest/gtest.h>
#include <memory>
#include <vector>

#include "kudu/consensus/consensus.pb.h"
Expand All @@ -29,6 +30,7 @@
namespace kudu {
namespace consensus {

using std::unique_ptr;
using std::vector;

// TODO: Share a test harness with ConsensusMetadataTest?
Expand All @@ -52,7 +54,7 @@ class RaftConsensusStateTest : public KuduTest {
peer->set_permanent_uuid(fs_manager_.uuid());
peer->set_member_type(RaftPeerPB::VOTER);

gscoped_ptr<ConsensusMetadata> cmeta;
unique_ptr<ConsensusMetadata> cmeta;
ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, fs_manager_.uuid(),
config_, kMinimumTerm, &cmeta));
state_.reset(new ReplicaState(ConsensusOptions(), fs_manager_.uuid(), std::move(cmeta),
Expand Down
4 changes: 3 additions & 1 deletion src/kudu/consensus/raft_consensus_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.

#include <algorithm>
#include <memory>

#include "kudu/consensus/log_util.h"
#include "kudu/consensus/quorum_util.h"
Expand All @@ -33,6 +34,7 @@ namespace kudu {
namespace consensus {

using std::string;
using std::unique_ptr;
using strings::Substitute;
using strings::SubstituteAndAppend;

Expand All @@ -41,7 +43,7 @@ using strings::SubstituteAndAppend;
//////////////////////////////////////////////////

ReplicaState::ReplicaState(ConsensusOptions options, string peer_uuid,
gscoped_ptr<ConsensusMetadata> cmeta,
unique_ptr<ConsensusMetadata> cmeta,
ReplicaTransactionFactory* txn_factory)
: options_(std::move(options)),
peer_uuid_(std::move(peer_uuid)),
Expand Down
Loading

0 comments on commit 28a9902

Please sign in to comment.