consensus: fix some clang-tidy warnings This fixes most of the clang-tidy warnings in the consensus module. I'm preparing to do some refactoring in the module, so I figured it was better to do the tidy ahead of time rather than fighting the clang-tidy bot when I moved some code.
Change-Id: Ieaaafe0bbc6b809b379f25e2076453dea973a37f Reviewed-on: http://gerrit.cloudera.org:8080/4454 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c5b07fa8 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c5b07fa8 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c5b07fa8 Branch: refs/heads/master Commit: c5b07fa8ff73bc2d26668946dc6e7e677d549d22 Parents: 3cc4cda Author: Todd Lipcon <[email protected]> Authored: Mon Sep 19 14:52:53 2016 -0700 Committer: Todd Lipcon <[email protected]> Committed: Fri Sep 23 19:09:04 2016 +0000 ---------------------------------------------------------------------- src/kudu/consensus/consensus-test-util.h | 10 +-- src/kudu/consensus/consensus.cc | 2 +- src/kudu/consensus/consensus_meta-test.cc | 2 +- src/kudu/consensus/consensus_meta.h | 6 +- src/kudu/consensus/consensus_peers-test.cc | 1 - src/kudu/consensus/consensus_peers.cc | 3 +- src/kudu/consensus/consensus_peers.h | 6 +- src/kudu/consensus/consensus_queue-test.cc | 6 +- src/kudu/consensus/consensus_queue.cc | 25 +++--- src/kudu/consensus/consensus_queue.h | 6 +- src/kudu/consensus/leader_election-test.cc | 4 +- src/kudu/consensus/leader_election.cc | 2 +- src/kudu/consensus/leader_election.h | 4 - src/kudu/consensus/log-test-base.h | 91 ++++++++++---------- src/kudu/consensus/log-test.cc | 15 ++-- src/kudu/consensus/log.cc | 46 +++++----- src/kudu/consensus/log.h | 17 ++-- src/kudu/consensus/log_anchor_registry.cc | 4 +- src/kudu/consensus/log_anchor_registry.h | 4 +- src/kudu/consensus/log_cache-test.cc | 2 +- src/kudu/consensus/log_cache.cc | 8 +- src/kudu/consensus/log_cache.h | 2 +- src/kudu/consensus/log_index.cc | 10 +-- src/kudu/consensus/log_metrics.h | 2 +- src/kudu/consensus/log_reader.cc | 9 +- src/kudu/consensus/log_reader.h | 15 ++-- src/kudu/consensus/log_util.h | 2 +- src/kudu/consensus/mt-log-test.cc | 4 +- src/kudu/consensus/opid_util.cc | 18 ++-- src/kudu/consensus/peer_manager.cc | 8 +- src/kudu/consensus/peer_manager.h | 4 +- src/kudu/consensus/quorum_util.cc | 8 +- src/kudu/consensus/raft_consensus-test.cc | 6 +- src/kudu/consensus/raft_consensus.cc | 25 +++--- src/kudu/consensus/raft_consensus.h | 2 +- .../consensus/raft_consensus_quorum-test.cc | 9 +- src/kudu/consensus/raft_consensus_state-test.cc | 5 +- src/kudu/consensus/raft_consensus_state.h | 5 +- src/kudu/tablet/tablet_bootstrap-test.cc | 8 -- 39 files changed, 188 insertions(+), 218 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus-test-util.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus-test-util.h b/src/kudu/consensus/consensus-test-util.h index b6fc201..f45783f 100644 --- a/src/kudu/consensus/consensus-test-util.h +++ b/src/kudu/consensus/consensus-test-util.h @@ -56,7 +56,7 @@ namespace consensus { using log::Log; using strings::Substitute; -static gscoped_ptr<ReplicateMsg> CreateDummyReplicate(int term, +inline gscoped_ptr<ReplicateMsg> CreateDummyReplicate(int term, int index, const Timestamp& timestamp, int payload_size) { @@ -72,7 +72,7 @@ static gscoped_ptr<ReplicateMsg> CreateDummyReplicate(int term, } // Returns RaftPeerPB with given UUID and obviously-fake hostname / port combo. -RaftPeerPB FakeRaftPeerPB(const std::string& uuid) { +inline RaftPeerPB FakeRaftPeerPB(const std::string& uuid) { RaftPeerPB peer_pb; peer_pb.set_permanent_uuid(uuid); peer_pb.mutable_last_known_addr()->set_host(Substitute("$0-fake-hostname", CURRENT_TEST_NAME())); @@ -85,7 +85,7 @@ RaftPeerPB FakeRaftPeerPB(const std::string& uuid) { // An operation will only be considered done (TestOperationStatus::IsDone() // will become true) once at least 'n_majority' peers have called // TestOperationStatus::AckPeer(). -static inline void AppendReplicateMessagesToQueue( +inline void AppendReplicateMessagesToQueue( PeerMessageQueue* queue, const scoped_refptr<server::Clock>& clock, int first, @@ -101,7 +101,7 @@ static inline void AppendReplicateMessagesToQueue( } // Builds a configuration of 'num' voters. -RaftConfigPB BuildRaftConfigPBForTests(int num) { +inline RaftConfigPB BuildRaftConfigPBForTests(int num) { RaftConfigPB raft_config; for (int i = 0; i < num; i++) { RaftPeerPB* peer_pb = raft_config.add_peers(); @@ -644,7 +644,7 @@ class MockTransactionFactory : public ReplicaTransactionFactory { // A transaction factory for tests, usually this is implemented by TabletPeer. class TestTransactionFactory : public ReplicaTransactionFactory { public: - explicit TestTransactionFactory(Log* log) : consensus_(NULL), + explicit TestTransactionFactory(Log* log) : consensus_(nullptr), log_(log) { CHECK_OK(ThreadPoolBuilder("test-txn-factory").set_max_threads(1).Build(&pool_)); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus.cc b/src/kudu/consensus/consensus.cc index d93c25b..ee3a398 100644 --- a/src/kudu/consensus/consensus.cc +++ b/src/kudu/consensus/consensus.cc @@ -86,7 +86,7 @@ const shared_ptr<Consensus::ConsensusFaultHooks>& Consensus::GetFaultHooks() con } Status Consensus::ExecuteHook(HookPoint point) { - if (PREDICT_FALSE(fault_hooks_.get() != nullptr)) { + if (PREDICT_FALSE(fault_hooks_ != nullptr)) { switch (point) { case Consensus::PRE_START: return fault_hooks_->PreStart(); case Consensus::POST_START: return fault_hooks_->PostStart(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/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 6c95199..7d5f652 100644 --- a/src/kudu/consensus/consensus_meta-test.cc +++ b/src/kudu/consensus/consensus_meta-test.cc @@ -255,7 +255,7 @@ TEST_F(ConsensusMetadataTest, TestMergeCommittedConsensusStatePB) { ASSERT_OK(ConsensusMetadata::Create(&fs_manager_, kTabletId, "e", committed_config, 1, &cmeta)); - uuids.push_back("e"); + uuids.emplace_back("e"); RaftConfigPB pending_config = BuildConfig(uuids); cmeta->set_pending_config(pending_config); cmeta->set_leader_uuid("e"); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_meta.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_meta.h b/src/kudu/consensus/consensus_meta.h index 9b014fa..7a6b8f0 100644 --- a/src/kudu/consensus/consensus_meta.h +++ b/src/kudu/consensus/consensus_meta.h @@ -17,8 +17,8 @@ #ifndef KUDU_CONSENSUS_CONSENSUS_META_H_ #define KUDU_CONSENSUS_CONSENSUS_META_H_ +#include <cstdint> #include <memory> -#include <stdint.h> #include <string> #include "kudu/consensus/metadata.pb.h" @@ -63,7 +63,7 @@ class ConsensusMetadata { const std::string& peer_uuid, const RaftConfigPB& config, int64_t current_term, - std::unique_ptr<ConsensusMetadata>* cmeta); + std::unique_ptr<ConsensusMetadata>* cmeta_out); // Load a ConsensusMetadata object from disk. // Returns Status::NotFound if the file could not be found. May return other @@ -71,7 +71,7 @@ class ConsensusMetadata { static Status Load(FsManager* fs_manager, const std::string& tablet_id, const std::string& peer_uuid, - std::unique_ptr<ConsensusMetadata>* cmeta); + std::unique_ptr<ConsensusMetadata>* cmeta_out); // Delete the ConsensusMetadata file associated with the given tablet from // disk. http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_peers-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_peers-test.cc b/src/kudu/consensus/consensus_peers-test.cc index 44927e5..5a9d48d 100644 --- a/src/kudu/consensus/consensus_peers-test.cc +++ b/src/kudu/consensus/consensus_peers-test.cc @@ -38,7 +38,6 @@ namespace consensus { using log::Log; using log::LogOptions; -using log::LogAnchorRegistry; const char* kTabletId = "test-peers-tablet"; const char* kLeaderUuid = "peer-0"; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_peers.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_peers.cc b/src/kudu/consensus/consensus_peers.cc index 6776b50..819f139 100644 --- a/src/kudu/consensus/consensus_peers.cc +++ b/src/kudu/consensus/consensus_peers.cc @@ -72,7 +72,6 @@ TAG_FLAG(enable_tablet_copy, unsafe); namespace kudu { namespace consensus { -using log::Log; using std::shared_ptr; using rpc::Messenger; using rpc::RpcController; @@ -467,7 +466,7 @@ Status SetPermanentUuidForRemotePeer(const shared_ptr<Messenger>& messenger, MonoTime now = MonoTime::Now(); if (now < deadline) { int64_t remaining_ms = (deadline - now).ToMilliseconds(); - int64_t base_delay_ms = 1 << (attempt + 3); // 1st retry delayed 2^4 ms, 2nd 2^5, etc.. + int64_t base_delay_ms = 1LL << (attempt + 3); // 1st retry delayed 2^4 ms, 2nd 2^5, etc.. int64_t jitter_ms = rand() % 50; // Add up to 50ms of additional random delay. int64_t delay_ms = std::min<int64_t>(base_delay_ms + jitter_ms, remaining_ms); VLOG(1) << "Sleeping " << delay_ms << " ms. before retrying to get uuid from remote peer..."; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_peers.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_peers.h b/src/kudu/consensus/consensus_peers.h index 286209b..c1a10cc 100644 --- a/src/kudu/consensus/consensus_peers.h +++ b/src/kudu/consensus/consensus_peers.h @@ -112,10 +112,10 @@ class Peer { Status Init(); // Signals that this peer has a new request to replicate/store. - // 'force_if_queue_empty' indicates whether the peer should force + // 'even_if_queue_empty' indicates whether the peer should force // send the request even if the queue is empty. This is used for // status-only requests. - Status SignalRequest(bool force_if_queue_empty = false); + Status SignalRequest(bool even_if_queue_empty = false); const RaftPeerPB& peer_pb() const { return peer_pb_; } @@ -145,7 +145,7 @@ class Peer { gscoped_ptr<Peer>* peer); private: - Peer(const RaftPeerPB& peer, std::string tablet_id, std::string leader_uuid, + Peer(const RaftPeerPB& peer_pb, std::string tablet_id, std::string leader_uuid, gscoped_ptr<PeerProxy> proxy, PeerMessageQueue* queue, ThreadPool* thread_pool); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_queue-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_queue-test.cc b/src/kudu/consensus/consensus_queue-test.cc index 69e99c7..3a939b5 100644 --- a/src/kudu/consensus/consensus_queue-test.cc +++ b/src/kudu/consensus/consensus_queue-test.cc @@ -20,13 +20,13 @@ #include "kudu/common/schema.h" #include "kudu/common/wire_protocol-test-util.h" -#include "kudu/consensus/consensus_queue.h" #include "kudu/consensus/consensus-test-util.h" +#include "kudu/consensus/consensus_queue.h" +#include "kudu/consensus/log-test-base.h" #include "kudu/consensus/log.h" #include "kudu/consensus/log_anchor_registry.h" -#include "kudu/consensus/log_util.h" #include "kudu/consensus/log_reader.h" -#include "kudu/consensus/log-test-base.h" +#include "kudu/consensus/log_util.h" #include "kudu/fs/fs_manager.h" #include "kudu/server/hybrid_clock.h" #include "kudu/util/metrics.h" http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_queue.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_queue.cc b/src/kudu/consensus/consensus_queue.cc index 0cafec4..b153bdd 100644 --- a/src/kudu/consensus/consensus_queue.cc +++ b/src/kudu/consensus/consensus_queue.cc @@ -35,10 +35,10 @@ #include "kudu/gutil/dynamic_annotations.h" #include "kudu/gutil/map-util.h" #include "kudu/gutil/stl_util.h" +#include "kudu/gutil/strings/human_readable.h" #include "kudu/gutil/strings/join.h" -#include "kudu/gutil/strings/substitute.h" #include "kudu/gutil/strings/strcat.h" -#include "kudu/gutil/strings/human_readable.h" +#include "kudu/gutil/strings/substitute.h" #include "kudu/util/fault_injection.h" #include "kudu/util/flag_tags.h" #include "kudu/util/locks.h" @@ -69,7 +69,6 @@ namespace kudu { namespace consensus { using log::Log; -using rpc::Messenger; using strings::Substitute; METRIC_DEFINE_gauge_int64(tablet, majority_done_ops, "Leader Operations Acked by Majority", @@ -377,17 +376,17 @@ Status PeerMessageQueue::RequestForPeer(const string& uuid, return s; // IsIncomplete() means that we tried to read beyond the head of the log // (in the future). See KUDU-1078. - } else if (s.IsIncomplete()) { + } + if (s.IsIncomplete()) { LOG_WITH_PREFIX_UNLOCKED(ERROR) << "Error trying to read ahead of the log " << "while preparing peer request: " << s.ToString() << ". Destination peer: " << peer->ToString(); return s; - } else { - LOG_WITH_PREFIX_UNLOCKED(FATAL) << "Error reading the log while preparing peer request: " - << s.ToString() << ". Destination peer: " - << peer->ToString(); } + LOG_WITH_PREFIX_UNLOCKED(FATAL) << "Error reading the log while preparing peer request: " + << s.ToString() << ". Destination peer: " + << peer->ToString(); } // We use AddAllocated rather than copy, because we pin the log cache at the @@ -463,11 +462,11 @@ void PeerMessageQueue::AdvanceQueueWatermark(const char* type, const OpId& replicated_before, const OpId& replicated_after, int num_peers_required, - const TrackedPeer* peer) { + const TrackedPeer* who_caused) { if (VLOG_IS_ON(2)) { VLOG_WITH_PREFIX_UNLOCKED(2) << "Updating " << type << " watermark: " - << "Peer (" << peer->ToString() << ") changed from " + << "Peer (" << who_caused->ToString() << ") changed from " << replicated_before << " to " << replicated_after << ". " << "Current value: " << *watermark; } @@ -758,7 +757,7 @@ void PeerMessageQueue::ResponseFromPeer(const std::string& peer_uuid, } } -PeerMessageQueue::TrackedPeer PeerMessageQueue::GetTrackedPeerForTests(string uuid) { +PeerMessageQueue::TrackedPeer PeerMessageQueue::GetTrackedPeerForTests(const string& uuid) { std::lock_guard<simple_spinlock> scoped_lock(queue_lock_); TrackedPeer* tracked = FindOrDie(peers_map_, uuid); return *tracked; @@ -884,10 +883,10 @@ bool PeerMessageQueue::IsOpInLog(const OpId& desired_op) const { return false; // Unreachable; here to squelch GCC warning. } -void PeerMessageQueue::NotifyObserversOfCommitIndexChange(int64_t commit_index) { +void PeerMessageQueue::NotifyObserversOfCommitIndexChange(int64_t new_commit_index) { WARN_NOT_OK(observers_pool_->SubmitClosure( Bind(&PeerMessageQueue::NotifyObserversOfCommitIndexChangeTask, - Unretained(this), commit_index)), + Unretained(this), new_commit_index)), LogPrefixUnlocked() + "Unable to notify RaftConsensus of " "commit index change."); } http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/consensus_queue.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/consensus_queue.h b/src/kudu/consensus/consensus_queue.h index 43d73d7..abcc089 100644 --- a/src/kudu/consensus/consensus_queue.h +++ b/src/kudu/consensus/consensus_queue.h @@ -156,10 +156,10 @@ class PeerMessageQueue { virtual void SetNonLeaderMode(); // Makes the queue track this peer. - virtual void TrackPeer(const std::string& peer_uuid); + virtual void TrackPeer(const std::string& uuid); // Makes the queue untrack this peer. - virtual void UntrackPeer(const std::string& peer_uuid); + virtual void UntrackPeer(const std::string& uuid); // Appends a single message to be replicated to the peers. // Returns OK unless the message could not be added to the queue for some @@ -248,7 +248,7 @@ class PeerMessageQueue { // Returns a copy of the TrackedPeer with 'uuid' or crashes if the peer is // not being tracked. - virtual TrackedPeer GetTrackedPeerForTests(std::string uuid); + virtual TrackedPeer GetTrackedPeerForTests(const std::string& uuid); virtual std::string ToString() const; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/leader_election-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/leader_election-test.cc b/src/kudu/consensus/leader_election-test.cc index 93fa89f..3f65511 100644 --- a/src/kudu/consensus/leader_election-test.cc +++ b/src/kudu/consensus/leader_election-test.cc @@ -42,7 +42,7 @@ namespace { const int kLeaderElectionTimeoutSecs = 10; // Generate list of voter uuids. -static vector<string> GenVoterUUIDs(int num_voters) { +vector<string> GenVoterUUIDs(int num_voters) { vector<string> voter_uuids; for (int i = 0; i < num_voters; i++) { voter_uuids.push_back(Substitute("peer-$0", i)); @@ -266,7 +266,7 @@ TEST_F(LeaderElectionTest, TestPerfectElection) { for (int num_voters : config_sizes) { LOG(INFO) << "Testing election with config size of " << num_voters; int majority_size = (num_voters / 2) + 1; - ConsensusTerm election_term = 10 + num_voters; // Just to be able to differentiate. + ConsensusTerm election_term = 10L + num_voters; // Just to be able to differentiate. InitUUIDs(num_voters); InitNoOpPeerProxies(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/leader_election.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/leader_election.cc b/src/kudu/consensus/leader_election.cc index 92723d0..1a40e6d 100644 --- a/src/kudu/consensus/leader_election.cc +++ b/src/kudu/consensus/leader_election.cc @@ -160,7 +160,7 @@ LeaderElection::LeaderElection(const RaftConfigPB& config, : has_responded_(false), request_(request), vote_counter_(std::move(vote_counter)), - timeout_(std::move(timeout)), + timeout_(timeout), decision_callback_(std::move(decision_callback)) { for (const RaftPeerPB& peer : config.peers()) { if (request.candidate_uuid() == peer.permanent_uuid()) continue; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/leader_election.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/leader_election.h b/src/kudu/consensus/leader_election.h index 394bca4..289215f 100644 --- a/src/kudu/consensus/leader_election.h +++ b/src/kudu/consensus/leader_election.h @@ -35,10 +35,6 @@ namespace kudu { class Status; -namespace metadata { -class RaftPeerPB; -} - namespace rpc { class Messenger; class RpcController; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log-test-base.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log-test-base.h b/src/kudu/consensus/log-test-base.h index ca19bbd..2a862d5 100644 --- a/src/kudu/consensus/log-test-base.h +++ b/src/kudu/consensus/log-test-base.h @@ -22,9 +22,9 @@ #include <glog/logging.h> #include <gtest/gtest.h> +#include <string> #include <utility> #include <vector> -#include <string> #include "kudu/common/timestamp.h" #include "kudu/common/wire_protocol-test-util.h" @@ -66,20 +66,20 @@ using tablet::TxResultPB; using tablet::OperationResultPB; using tablet::MemStoreTargetPB; -const char* kTestTable = "test-log-table"; -const char* kTestTableId = "test-log-table-id"; -const char* kTestTablet = "test-log-tablet"; -const bool APPEND_SYNC = true; -const bool APPEND_ASYNC = false; +constexpr char kTestTable[] = "test-log-table"; +constexpr char kTestTableId[] = "test-log-table-id"; +constexpr char kTestTablet[] = "test-log-tablet"; +constexpr bool APPEND_SYNC = true; +constexpr bool APPEND_ASYNC = false; // Append a single batch of 'count' NoOps to the log. // If 'size' is not NULL, increments it by the expected increase in log size. // Increments 'op_id''s index once for each operation logged. -static Status AppendNoOpsToLogSync(const scoped_refptr<Clock>& clock, - Log* log, - OpId* op_id, - int count, - int* size = NULL) { +inline Status AppendNoOpsToLogSync(const scoped_refptr<Clock>& clock, + Log* log, + OpId* op_id, + int count, + int* size = NULL) { vector<consensus::ReplicateRefPtr> replicates; for (int i = 0; i < count; i++) { @@ -112,13 +112,44 @@ static Status AppendNoOpsToLogSync(const scoped_refptr<Clock>& clock, return s.Wait(); } -static Status AppendNoOpToLogSync(const scoped_refptr<Clock>& clock, - Log* log, - OpId* op_id, - int* size = NULL) { +inline Status AppendNoOpToLogSync(const scoped_refptr<Clock>& clock, + Log* log, + OpId* op_id, + int* size = NULL) { return AppendNoOpsToLogSync(clock, log, op_id, 1, size); } + +// Corrupts the last segment of the provided log by either truncating it +// or modifying a byte at the given offset. +enum CorruptionType { + TRUNCATE_FILE, + FLIP_BYTE +}; + +inline Status CorruptLogFile(Env* env, const string& log_path, + CorruptionType type, int corruption_offset) { + faststring buf; + RETURN_NOT_OK_PREPEND(ReadFileToString(env, log_path, &buf), + "Couldn't read log"); + + switch (type) { + case TRUNCATE_FILE: + buf.resize(corruption_offset); + break; + case FLIP_BYTE: + CHECK_LT(corruption_offset, buf.size()); + buf[corruption_offset] ^= 0xff; + break; + } + + // Rewrite the file with the corrupt log. + RETURN_NOT_OK_PREPEND(WriteStringToFile(env, Slice(buf), log_path), + "Couldn't rewrite corrupt log file"); + + return Status::OK(); +} + class LogTestBase : public KuduTest { public: @@ -357,36 +388,6 @@ class LogTestBase : public KuduTest { scoped_refptr<Clock> clock_; }; -// Corrupts the last segment of the provided log by either truncating it -// or modifying a byte at the given offset. -enum CorruptionType { - TRUNCATE_FILE, - FLIP_BYTE -}; - -Status CorruptLogFile(Env* env, const string& log_path, - CorruptionType type, int corruption_offset) { - faststring buf; - RETURN_NOT_OK_PREPEND(ReadFileToString(env, log_path, &buf), - "Couldn't read log"); - - switch (type) { - case TRUNCATE_FILE: - buf.resize(corruption_offset); - break; - case FLIP_BYTE: - CHECK_LT(corruption_offset, buf.size()); - buf[corruption_offset] ^= 0xff; - break; - } - - // Rewrite the file with the corrupt log. - RETURN_NOT_OK_PREPEND(WriteStringToFile(env, Slice(buf), log_path), - "Couldn't rewrite corrupt log file"); - - return Status::OK(); -} - } // namespace log } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log-test.cc b/src/kudu/consensus/log-test.cc index 2042c7d..c755dd8 100644 --- a/src/kudu/consensus/log-test.cc +++ b/src/kudu/consensus/log-test.cc @@ -47,9 +47,6 @@ using std::shared_ptr; using consensus::MakeOpId; using strings::Substitute; -extern const char* kTestTable; -extern const char* kTestTablet; - struct TestLogSequenceElem { enum ElemType { REPLICATE, @@ -110,7 +107,7 @@ class LogTest : public LogTestBase { LogSegmentFooterPB footer; footer.set_num_entries(10); footer.set_min_replicate_index(first_repl_index); - footer.set_max_replicate_index(first_repl_index + 9); + footer.set_max_replicate_index(first_repl_index + 9L); RETURN_NOT_OK(readable_segment->Init(header, footer, 0)); RETURN_NOT_OK(reader->AppendSegment(readable_segment)); @@ -131,7 +128,7 @@ class LogTest : public LogTestBase { }; void DoCorruptionTest(CorruptionType type, CorruptionPosition place, - Status expected_status, int expected_entries); + const Status& expected_status, int expected_entries); }; @@ -275,7 +272,7 @@ TEST_F(LogTest, TestBlankLogFile) { } void LogTest::DoCorruptionTest(CorruptionType type, CorruptionPosition place, - Status expected_status, int expected_entries) { + const Status& expected_status, int expected_entries) { const int kNumEntries = 4; ASSERT_OK(BuildLog()); OpId op_id = MakeOpId(1, 1); @@ -415,7 +412,7 @@ TEST_F(LogTest, TestWriteAndReadToAndFromInProgressSegment) { repl->set_timestamp(0L); // Entries are prefixed with a header. - int single_entry_size = batch.ByteSize() + kEntryHeaderSize; + int64_t single_entry_size = batch.ByteSize() + kEntryHeaderSize; int written_entries_size = header_size; ASSERT_OK(AppendNoOps(&op_id, kNumEntries, &written_entries_size)); @@ -705,7 +702,7 @@ TEST_F(LogTest, TestWriteManyBatches) { ASSERT_OK(LogReader::Open(fs_manager_.get(), NULL, kTestTablet, NULL, &reader)); ASSERT_OK(reader->GetSegmentsSnapshot(&segments)); - for (const scoped_refptr<ReadableLogSegment> entry : segments) { + for (const scoped_refptr<ReadableLogSegment>& entry : segments) { STLDeleteElements(&entries_); ASSERT_OK(entry->ReadEntries(&entries_)); num_entries += entries_.size(); @@ -800,7 +797,7 @@ std::ostream& operator<<(std::ostream& os, const TestLogSequenceElem& elem) { void LogTest::GenerateTestSequence(Random* rng, int seq_len, vector<TestLogSequenceElem>* ops, vector<int64_t>* terms_by_index) { - terms_by_index->assign(seq_len + 1, -1); + terms_by_index->assign(seq_len + 1L, -1L); int64_t committed_index = 0; int64_t max_repl_index = 0; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log.cc b/src/kudu/consensus/log.cc index c5a9344..4a72048 100644 --- a/src/kudu/consensus/log.cc +++ b/src/kudu/consensus/log.cc @@ -18,8 +18,8 @@ #include "kudu/consensus/log.h" #include <algorithm> -#include <mutex> #include <limits> +#include <mutex> #include "kudu/common/wire_protocol.h" #include "kudu/consensus/log_index.h" @@ -294,7 +294,7 @@ Status Log::Open(const LogOptions &options, Log::Log(LogOptions options, FsManager* fs_manager, string log_path, string tablet_id, const Schema& schema, uint32_t schema_version, const scoped_refptr<MetricEntity>& metric_entity) - : options_(std::move(options)), + : options_(options), fs_manager_(fs_manager), log_dir_(std::move(log_path)), tablet_id_(std::move(tablet_id)), @@ -450,17 +450,17 @@ Status Log::AsyncAppend(LogEntryBatch* entry_batch, const StatusCallback& callba return Status::OK(); } -Status Log::AsyncAppendReplicates(const vector<ReplicateRefPtr>& msgs, +Status Log::AsyncAppendReplicates(const vector<ReplicateRefPtr>& replicates, const StatusCallback& callback) { gscoped_ptr<LogEntryBatchPB> batch; - CreateBatchFromAllocatedOperations(msgs, &batch); + CreateBatchFromAllocatedOperations(replicates, &batch); LogEntryBatch* reserved_entry_batch; RETURN_NOT_OK(Reserve(REPLICATE, std::move(batch), &reserved_entry_batch)); // If we're able to reserve set the vector of replicate scoped ptrs in // the LogEntryBatch. This will make sure there's a reference for each // replicate while we're appending. - reserved_entry_batch->SetReplicates(msgs); + reserved_entry_batch->SetReplicates(replicates); RETURN_NOT_OK(AsyncAppend(reserved_entry_batch, callback)); return Status::OK(); @@ -627,7 +627,7 @@ Status Log::Sync() { return Status::OK(); } -int GetPrefixSizeToGC(RetentionIndexes retention, const SegmentSequence& segments) { +int GetPrefixSizeToGC(RetentionIndexes retention_indexes, const SegmentSequence& segments) { int rem_segs = segments.size(); int prefix_size = 0; for (const scoped_refptr<ReadableLogSegment>& segment : segments) { @@ -639,13 +639,13 @@ int GetPrefixSizeToGC(RetentionIndexes retention, const SegmentSequence& segment int64_t seg_max_idx = segment->footer().max_replicate_index(); // If removing this segment would compromise durability, we cannot remove it. - if (seg_max_idx >= retention.for_durability) { + if (seg_max_idx >= retention_indexes.for_durability) { break; } // Check if removing this segment would compromise the ability to catch up a peer, // we should retain it, unless this would break the max_segments flag. - if (seg_max_idx >= retention.for_peers && + if (seg_max_idx >= retention_indexes.for_peers && rem_segs <= FLAGS_log_max_segments_to_retain) { break; } @@ -656,17 +656,17 @@ int GetPrefixSizeToGC(RetentionIndexes retention, const SegmentSequence& segment return prefix_size; } -Status Log::GetSegmentsToGCUnlocked(RetentionIndexes retention, +Status Log::GetSegmentsToGCUnlocked(RetentionIndexes retention_indexes, SegmentSequence* segments_to_gc) const { RETURN_NOT_OK(reader_->GetSegmentsSnapshot(segments_to_gc)); - segments_to_gc->resize(GetPrefixSizeToGC(retention, *segments_to_gc)); + segments_to_gc->resize(GetPrefixSizeToGC(retention_indexes, *segments_to_gc)); return Status::OK(); } -Status Log::Append(LogEntryPB* phys_entry) { +Status Log::Append(LogEntryPB* entry) { gscoped_ptr<LogEntryBatchPB> entry_batch_pb(new LogEntryBatchPB); - entry_batch_pb->mutable_entry()->AddAllocated(phys_entry); - LogEntryBatch entry_batch(phys_entry->type(), std::move(entry_batch_pb), 1); + entry_batch_pb->mutable_entry()->AddAllocated(entry); + LogEntryBatch entry_batch(entry->type(), std::move(entry_batch_pb), 1); entry_batch.state_ = LogEntryBatch::kEntryReserved; Status s = entry_batch.Serialize(); if (s.ok()) { @@ -701,12 +701,12 @@ void Log::GetLatestEntryOpId(consensus::OpId* op_id) const { } } -Status Log::GC(RetentionIndexes retention, int32_t* num_gced) { - CHECK_GE(retention.for_durability, 0); +Status Log::GC(RetentionIndexes retention_indexes, int32_t* num_gced) { + CHECK_GE(retention_indexes.for_durability, 0); VLOG(1) << "Running Log GC on " << log_dir_ << ": retaining " - "ops >= " << retention.for_durability << " for durability, " - "ops >= " << retention.for_peers << " for peers"; + "ops >= " << retention_indexes.for_durability << " for durability, " + "ops >= " << retention_indexes.for_peers << " for peers"; VLOG_TIMING(1, "Log GC") { SegmentSequence segments_to_delete; @@ -714,9 +714,9 @@ Status Log::GC(RetentionIndexes retention, int32_t* num_gced) { std::lock_guard<percpu_rwlock> l(state_lock_); CHECK_EQ(kLogWriting, log_state_); - RETURN_NOT_OK(GetSegmentsToGCUnlocked(retention, &segments_to_delete)); + RETURN_NOT_OK(GetSegmentsToGCUnlocked(retention_indexes, &segments_to_delete)); - if (segments_to_delete.size() == 0) { + if (segments_to_delete.empty()) { VLOG(1) << "No segments to delete."; *num_gced = 0; return Status::OK(); @@ -752,16 +752,16 @@ Status Log::GC(RetentionIndexes retention, int32_t* num_gced) { return Status::OK(); } -void Log::GetGCableDataSize(RetentionIndexes retention, int64_t* total_size) const { - CHECK_GE(retention.for_durability, 0); +void Log::GetGCableDataSize(RetentionIndexes retention_indexes, int64_t* total_size) const { + CHECK_GE(retention_indexes.for_durability, 0); SegmentSequence segments_to_delete; *total_size = 0; { shared_lock<rw_spinlock> l(state_lock_.get_lock()); CHECK_EQ(kLogWriting, log_state_); - Status s = GetSegmentsToGCUnlocked(retention, &segments_to_delete); + Status s = GetSegmentsToGCUnlocked(retention_indexes, &segments_to_delete); - if (!s.ok() || segments_to_delete.size() == 0) { + if (!s.ok() || segments_to_delete.empty()) { return; } } http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log.h b/src/kudu/consensus/log.h index 49623e4..a43de21 100644 --- a/src/kudu/consensus/log.h +++ b/src/kudu/consensus/log.h @@ -112,9 +112,9 @@ class Log : public RefCountedThreadSafe<Log> { gscoped_ptr<LogEntryBatchPB> entry_batch, LogEntryBatch** reserved_entry); - // Asynchronously appends 'entry' to the log. Once the append + // Asynchronously appends 'entry_batch' to the log. Once the append // completes and is synced, 'callback' will be invoked. - Status AsyncAppend(LogEntryBatch* entry, + Status AsyncAppend(LogEntryBatch* entry_batch, const StatusCallback& callback); // Synchronously append a new entry to the log. @@ -195,10 +195,10 @@ class Log : public RefCountedThreadSafe<Log> { // If successful, num_gced is set to the number of deleted log segments. // // This method is thread-safe. - Status GC(RetentionIndexes indexes, int* num_gced); + Status GC(RetentionIndexes retention_indexes, int* num_gced); // Computes the amount of bytes that would have been GC'd if Log::GC had been called. - void GetGCableDataSize(RetentionIndexes indexes, int64_t* total_size) const; + void GetGCableDataSize(RetentionIndexes retention_indexes, int64_t* total_size) const; // Returns a map of log index -> segment size, of all the segments that currently cannot be GCed // because of an anchor on the given 'idx_for_durability' log index. Note that, even if @@ -284,7 +284,7 @@ class Log : public RefCountedThreadSafe<Log> { // AppenderThread. If 'caller_owns_operation' is true, then the // 'operation' field of the entry will be released after the entry // is appended. - Status DoAppend(LogEntryBatch* entry); + Status DoAppend(LogEntryBatch* entry_batch); // Update footer_builder_ to reflect the log indexes seen in 'batch'. void UpdateFooterForBatch(LogEntryBatch* batch); @@ -302,7 +302,8 @@ class Log : public RefCountedThreadSafe<Log> { Status Sync(); // Helper method to get the segment sequence to GC based on the provided 'retention' struct. - Status GetSegmentsToGCUnlocked(RetentionIndexes retention, SegmentSequence* segments_to_gc) const; + Status GetSegmentsToGCUnlocked(RetentionIndexes retention_indexes, + SegmentSequence* segments_to_gc) const; LogEntryBatchQueue* entry_queue() { return &entry_batch_queue_; @@ -405,8 +406,8 @@ class Log : public RefCountedThreadSafe<Log> { // When default-constructed, starts with maximum indexes, indicating no // logs need to be retained for either purposes. struct RetentionIndexes { - RetentionIndexes(int64_t durability = std::numeric_limits<int64_t>::max(), - int64_t peers = std::numeric_limits<int64_t>::max()) + explicit RetentionIndexes(int64_t durability = std::numeric_limits<int64_t>::max(), + int64_t peers = std::numeric_limits<int64_t>::max()) : for_durability(durability), for_peers(peers) {} http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_anchor_registry.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_anchor_registry.cc b/src/kudu/consensus/log_anchor_registry.cc index 5d2f2f6..0243d54 100644 --- a/src/kudu/consensus/log_anchor_registry.cc +++ b/src/kudu/consensus/log_anchor_registry.cc @@ -27,7 +27,6 @@ namespace kudu { namespace log { using consensus::kInvalidOpIdIndex; -using std::pair; using std::string; using strings::Substitute; using strings::SubstituteAndAppend; @@ -125,9 +124,8 @@ Status LogAnchorRegistry::UnregisterUnlocked(LogAnchor* anchor) { anchors_.erase(iter); // No need for the iterator to remain valid since we return here. return Status::OK(); - } else { - ++iter; } + ++iter; } return Status::NotFound(Substitute("Anchor with index $0 and owner $1 not found", anchor->log_index, anchor->owner)); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_anchor_registry.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_anchor_registry.h b/src/kudu/consensus/log_anchor_registry.h index aaf5c8d..a85c3ea 100644 --- a/src/kudu/consensus/log_anchor_registry.h +++ b/src/kudu/consensus/log_anchor_registry.h @@ -17,9 +17,9 @@ #ifndef KUDU_CONSENSUS_LOG_ANCHOR_REGISTRY_ #define KUDU_CONSENSUS_LOG_ANCHOR_REGISTRY_ +#include <gtest/gtest_prod.h> #include <map> #include <string> -#include <gtest/gtest_prod.h> #include "kudu/gutil/macros.h" #include "kudu/gutil/ref_counted.h" @@ -66,7 +66,7 @@ class LogAnchorRegistry : public RefCountedThreadSafe<LogAnchorRegistry> { // Query the registry to find the earliest anchored log index in the registry. // Returns Status::NotFound if no anchors are currently active. - Status GetEarliestRegisteredLogIndex(int64_t* op_id); + Status GetEarliestRegisteredLogIndex(int64_t* log_index); // Simply returns the number of active anchors for use in debugging / tests. // This is _not_ a constant-time operation. http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_cache-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_cache-test.cc b/src/kudu/consensus/log_cache-test.cc index f19783b..28e6a32 100644 --- a/src/kudu/consensus/log_cache-test.cc +++ b/src/kudu/consensus/log_cache-test.cc @@ -61,7 +61,7 @@ class LogCacheTest : public KuduTest { kTestTablet, schema_, 0, // schema_version - NULL, + nullptr, &log_)); CloseAndReopenCache(MinimumOpId()); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_cache.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_cache.cc b/src/kudu/consensus/log_cache.cc index 25ecd1c..dcb4ba2 100644 --- a/src/kudu/consensus/log_cache.cc +++ b/src/kudu/consensus/log_cache.cc @@ -35,10 +35,10 @@ #include "kudu/gutil/strings/substitute.h" #include "kudu/util/debug-util.h" #include "kudu/util/flag_tags.h" -#include "kudu/util/mem_tracker.h" -#include "kudu/util/metrics.h" #include "kudu/util/locks.h" #include "kudu/util/logging.h" +#include "kudu/util/mem_tracker.h" +#include "kudu/util/metrics.h" DEFINE_int32(log_cache_size_limit_mb, 128, "The total per-tablet size of consensus entries which may be kept in memory. " @@ -80,8 +80,8 @@ LogCache::LogCache(const scoped_refptr<MetricEntity>& metric_entity, metrics_(metric_entity) { - const int64_t max_ops_size_bytes = FLAGS_log_cache_size_limit_mb * 1024 * 1024; - const int64_t global_max_ops_size_bytes = FLAGS_global_log_cache_size_limit_mb * 1024 * 1024; + const int64_t max_ops_size_bytes = FLAGS_log_cache_size_limit_mb * 1024L * 1024L; + const int64_t global_max_ops_size_bytes = FLAGS_global_log_cache_size_limit_mb * 1024L * 1024L; // Set up (or reuse) a tracker with the global limit. It is parented directly // to the root tracker so that it's always global. http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_cache.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_cache.h b/src/kudu/consensus/log_cache.h index 77f4d3d..b056230 100644 --- a/src/kudu/consensus/log_cache.h +++ b/src/kudu/consensus/log_cache.h @@ -109,7 +109,7 @@ class LogCache { // Return true if an operation with the given index has been written through // the cache. The operation may not necessarily be durable yet -- it could still be // en route to the log. - bool HasOpBeenWritten(int64_t log_index) const; + bool HasOpBeenWritten(int64_t index) const; // Evict any operations with op index <= 'index'. void EvictThroughOp(int64_t index); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_index.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_index.cc b/src/kudu/consensus/log_index.cc index e1f25b7..9cd0ce3 100644 --- a/src/kudu/consensus/log_index.cc +++ b/src/kudu/consensus/log_index.cc @@ -47,9 +47,9 @@ using std::string; using strings::Substitute; -#define RETRY_ON_EINTR(ret, expr) do { \ - ret = expr; \ -} while ((ret == -1) && (errno == EINTR)); +#define RETRY_ON_EINTR(ret, expr) do { \ + (ret) = (expr); \ + } while (((ret) == -1) && (errno == EINTR)); namespace kudu { namespace log { @@ -137,11 +137,11 @@ void LogIndex::IndexChunk::GetEntry(int entry_index, PhysicalEntry* ret) { memcpy(ret, mapping_ + sizeof(PhysicalEntry) * entry_index, sizeof(PhysicalEntry)); } -void LogIndex::IndexChunk::SetEntry(int entry_index, const PhysicalEntry& phys) { +void LogIndex::IndexChunk::SetEntry(int entry_index, const PhysicalEntry& entry) { DCHECK_GE(fd_, 0) << "Must Open() first"; DCHECK_LT(entry_index, kEntriesPerIndexChunk); - memcpy(mapping_ + sizeof(PhysicalEntry) * entry_index, &phys, sizeof(PhysicalEntry)); + memcpy(mapping_ + sizeof(PhysicalEntry) * entry_index, &entry, sizeof(PhysicalEntry)); } //////////////////////////////////////////////////////////// http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_metrics.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_metrics.h b/src/kudu/consensus/log_metrics.h index 22e712b..f29e58c 100644 --- a/src/kudu/consensus/log_metrics.h +++ b/src/kudu/consensus/log_metrics.h @@ -45,7 +45,7 @@ struct LogMetrics { // TODO extract and generalize this for all histogram metrics #define SCOPED_LATENCY_METRIC(_mtx, _h) \ - ScopedLatencyMetric _h##_metric(_mtx ? _mtx->_h.get() : NULL) + ScopedLatencyMetric _h##_metric((_mtx) ? (_mtx)->_h.get() : NULL) } // namespace log } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_reader.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_reader.cc b/src/kudu/consensus/log_reader.cc index 6b78f8a..4a86505 100644 --- a/src/kudu/consensus/log_reader.cc +++ b/src/kudu/consensus/log_reader.cc @@ -60,7 +60,6 @@ struct LogSegmentSeqnoComparator { using consensus::OpId; using consensus::ReplicateMsg; -using env_util::ReadFully; using std::shared_ptr; using strings::Substitute; @@ -175,10 +174,8 @@ Status LogReader::Init(const string& tablet_wal_path) { "Previous segment: seqno $0, path $1; Current segment: seqno $2, path $3", previous_seg_seqno, previous_seg_path, entry->header().sequence_number(), entry->path())); - previous_seg_seqno++; - } else { - previous_seg_seqno = entry->header().sequence_number(); } + previous_seg_seqno = entry->header().sequence_number(); previous_seg_path = entry->path(); RETURN_NOT_OK(AppendSegmentUnlocked(entry)); } @@ -260,8 +257,8 @@ Status LogReader::ReadBatchUsingIndexEntry(const LogIndexEntry& index_entry, return Status::OK(); } -Status LogReader::ReadReplicatesInRange(const int64_t starting_at, - const int64_t up_to, +Status LogReader::ReadReplicatesInRange(int64_t starting_at, + int64_t up_to, int64_t max_bytes_to_read, vector<ReplicateMsg*>* replicates) const { DCHECK_GT(starting_at, 0); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_reader.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_reader.h b/src/kudu/consensus/log_reader.h index b4666a7..d1fc322 100644 --- a/src/kudu/consensus/log_reader.h +++ b/src/kudu/consensus/log_reader.h @@ -85,8 +85,8 @@ class LogReader { // // Requires that a LogIndex was passed into LogReader::Open(). Status ReadReplicatesInRange( - const int64_t starting_at, - const int64_t up_to, + int64_t starting_at, + int64_t up_to, int64_t max_bytes_to_read, std::vector<consensus::ReplicateMsg*>* replicates) const; static const int kNoSizeLimit; @@ -122,8 +122,9 @@ class LogReader { // Used by the Log to add "empty" segments. Status AppendEmptySegment(const scoped_refptr<ReadableLogSegment>& segment); - // Removes segments with sequence numbers less than or equal to 'seg_seqno' from this reader. - Status TrimSegmentsUpToAndIncluding(int64_t seg_seqno); + // Removes segments with sequence numbers less than or equal to + // 'segment_sequence_number' from this reader. + Status TrimSegmentsUpToAndIncluding(int64_t segment_sequence_number); // Replaces the last segment in the reader with 'segment'. // Used to replace a segment that was still in the process of being written @@ -153,11 +154,11 @@ class LogReader { gscoped_ptr<LogEntryBatchPB>* batch) const; LogReader(FsManager* fs_manager, const scoped_refptr<LogIndex>& index, - std::string tablet_name, + std::string tablet_id, const scoped_refptr<MetricEntity>& metric_entity); - // Reads the headers of all segments in 'path_'. - Status Init(const std::string& path_); + // Reads the headers of all segments in 'tablet_wal_path'. + Status Init(const std::string& tablet_wal_path); // Initializes an 'empty' reader for tests, i.e. does not scan a path looking for segments. Status InitEmptyReaderForTests(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/log_util.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/log_util.h b/src/kudu/consensus/log_util.h index a4b2bae..f64aadd 100644 --- a/src/kudu/consensus/log_util.h +++ b/src/kudu/consensus/log_util.h @@ -372,7 +372,7 @@ class WritableLogSegment { // Appends the provided batch of data, including a header // and checksum. // Makes sure that the log segment has not been closed. - Status WriteEntryBatch(const Slice& entry_batch_data); + Status WriteEntryBatch(const Slice& data); // Makes sure the I/O buffers in the underlying writable file are flushed. Status Sync() { http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/mt-log-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/mt-log-test.cc b/src/kudu/consensus/mt-log-test.cc index 7f590e2..7407614 100644 --- a/src/kudu/consensus/mt-log-test.cc +++ b/src/kudu/consensus/mt-log-test.cc @@ -68,8 +68,6 @@ class CustomLatchCallback : public RefCountedThreadSafe<CustomLatchCallback> { } // anonymous namespace -extern const char *kTestTablet; - class MultiThreadedLogTest : public LogTestBase { public: MultiThreadedLogTest() @@ -158,7 +156,7 @@ TEST_F(MultiThreadedLogTest, TestAppends) { ASSERT_OK(log_->Close()); shared_ptr<LogReader> reader; - ASSERT_OK(LogReader::Open(fs_manager_.get(), NULL, kTestTablet, NULL, &reader)); + ASSERT_OK(LogReader::Open(fs_manager_.get(), nullptr, kTestTablet, nullptr, &reader)); SegmentSequence segments; ASSERT_OK(reader->GetSegmentsSnapshot(&segments)); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/opid_util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/opid_util.cc b/src/kudu/consensus/opid_util.cc index dac5169..7b43069 100644 --- a/src/kudu/consensus/opid_util.cc +++ b/src/kudu/consensus/opid_util.cc @@ -32,13 +32,13 @@ const int64_t kMinimumTerm = 0; const int64_t kMinimumOpIdIndex = 0; const int64_t kInvalidOpIdIndex = -1; -int OpIdCompare(const OpId& first, const OpId& second) { - DCHECK(first.IsInitialized()); - DCHECK(second.IsInitialized()); - if (PREDICT_TRUE(first.term() == second.term())) { - return first.index() < second.index() ? -1 : first.index() == second.index() ? 0 : 1; +int OpIdCompare(const OpId& left, const OpId& right) { + DCHECK(left.IsInitialized()); + DCHECK(right.IsInitialized()); + if (PREDICT_TRUE(left.term() == right.term())) { + return left.index() < right.index() ? -1 : left.index() == right.index() ? 0 : 1; } - return first.term() < second.term() ? -1 : 1; + return left.term() < right.term() ? -1 : 1; } bool OpIdEquals(const OpId& left, const OpId& right) { @@ -130,11 +130,11 @@ std::ostream& operator<<(std::ostream& os, const consensus::OpId& op_id) { return os; } -std::string OpIdToString(const OpId& op_id) { - if (!op_id.IsInitialized()) { +std::string OpIdToString(const OpId& id) { + if (!id.IsInitialized()) { return "<uninitialized op>"; } - return strings::Substitute("$0.$1", op_id.term(), op_id.index()); + return strings::Substitute("$0.$1", id.term(), id.index()); } std::string OpsRangeString(const ConsensusRequestPB& req) { http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/peer_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/peer_manager.cc b/src/kudu/consensus/peer_manager.cc index cb7df2f..6d39ff5 100644 --- a/src/kudu/consensus/peer_manager.cc +++ b/src/kudu/consensus/peer_manager.cc @@ -32,14 +32,14 @@ namespace consensus { using log::Log; using strings::Substitute; -PeerManager::PeerManager(const std::string tablet_id, - const std::string local_uuid, +PeerManager::PeerManager(std::string tablet_id, + std::string local_uuid, PeerProxyFactory* peer_proxy_factory, PeerMessageQueue* queue, ThreadPool* request_thread_pool, const scoped_refptr<log::Log>& log) - : tablet_id_(tablet_id), - local_uuid_(local_uuid), + : tablet_id_(std::move(tablet_id)), + local_uuid_(std::move(local_uuid)), peer_proxy_factory_(peer_proxy_factory), queue_(queue), thread_pool_(request_thread_pool), http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/peer_manager.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/peer_manager.h b/src/kudu/consensus/peer_manager.h index c1dc86f..560f27f 100644 --- a/src/kudu/consensus/peer_manager.h +++ b/src/kudu/consensus/peer_manager.h @@ -51,8 +51,8 @@ class PeerManager { // // 'request_thread_pool' is the pool used to construct requests to send // to the peers. - PeerManager(const std::string tablet_id, - const std::string local_uuid, + PeerManager(std::string tablet_id, + std::string local_uuid, PeerProxyFactory* peer_proxy_factory, PeerMessageQueue* queue, ThreadPool* request_thread_pool, http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/quorum_util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc index 44dc306..5db7fff 100644 --- a/src/kudu/consensus/quorum_util.cc +++ b/src/kudu/consensus/quorum_util.cc @@ -103,17 +103,17 @@ int MajoritySize(int num_voters) { return (num_voters / 2) + 1; } -RaftPeerPB::Role GetConsensusRole(const std::string& permanent_uuid, +RaftPeerPB::Role GetConsensusRole(const std::string& uuid, const ConsensusStatePB& cstate) { - if (cstate.leader_uuid() == permanent_uuid) { - if (IsRaftConfigVoter(permanent_uuid, cstate.config())) { + if (cstate.leader_uuid() == uuid) { + if (IsRaftConfigVoter(uuid, cstate.config())) { return RaftPeerPB::LEADER; } return RaftPeerPB::NON_PARTICIPANT; } for (const RaftPeerPB& peer : cstate.config().peers()) { - if (peer.permanent_uuid() == permanent_uuid) { + if (peer.permanent_uuid() == uuid) { switch (peer.member_type()) { case RaftPeerPB::VOTER: return RaftPeerPB::FOLLOWER; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/raft_consensus-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus-test.cc b/src/kudu/consensus/raft_consensus-test.cc index 0aa77b1..147d626 100644 --- a/src/kudu/consensus/raft_consensus-test.cc +++ b/src/kudu/consensus/raft_consensus-test.cc @@ -50,11 +50,9 @@ using log::LogOptions; using ::testing::_; using ::testing::AnyNumber; using ::testing::AtLeast; -using ::testing::Eq; using ::testing::InSequence; using ::testing::Invoke; using ::testing::Mock; -using ::testing::Property; using ::testing::Return; const char* kTestTablet = "TestTablet"; @@ -189,7 +187,7 @@ class RaftConsensusTest : public KuduTest { kTestTablet, schema_, 0, // schema_version - NULL, + nullptr, &log_)); queue_ = new MockQueue(metric_entity_, log_.get()); @@ -361,7 +359,7 @@ TEST_F(RaftConsensusTest, TestPendingTransactions) { for (int i = 0; i < 10; i++) { auto replicate = new ReplicateMsg(); replicate->set_op_type(NO_OP); - info.last_id.set_index(100 + i); + info.last_id.set_index(100L + i); replicate->mutable_id()->CopyFrom(info.last_id); info.orphaned_replicates.push_back(replicate); } http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/raft_consensus.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc index 3b039d3..aaaf228 100644 --- a/src/kudu/consensus/raft_consensus.cc +++ b/src/kudu/consensus/raft_consensus.cc @@ -207,7 +207,7 @@ scoped_refptr<RaftConsensus> RaftConsensus::Create( RaftConsensus::RaftConsensus( const ConsensusOptions& options, unique_ptr<ConsensusMetadata> cmeta, - gscoped_ptr<PeerProxyFactory> proxy_factory, + gscoped_ptr<PeerProxyFactory> peer_proxy_factory, gscoped_ptr<PeerMessageQueue> queue, gscoped_ptr<PeerManager> peer_manager, gscoped_ptr<ThreadPool> thread_pool, @@ -221,7 +221,7 @@ RaftConsensus::RaftConsensus( : thread_pool_(std::move(thread_pool)), log_(log), clock_(clock), - peer_proxy_factory_(std::move(proxy_factory)), + peer_proxy_factory_(std::move(peer_proxy_factory)), peer_manager_(std::move(peer_manager)), queue_(std::move(queue)), rng_(GetRandomSeed32()), @@ -360,7 +360,8 @@ Status RaftConsensus::StartElection(ElectionMode mode) { if (active_role == RaftPeerPB::LEADER) { LOG_WITH_PREFIX_UNLOCKED(INFO) << "Not starting election -- already leader"; return Status::OK(); - } else if (PREDICT_FALSE(active_role == RaftPeerPB::NON_PARTICIPANT)) { + } + if (PREDICT_FALSE(active_role == RaftPeerPB::NON_PARTICIPANT)) { SnoozeFailureDetectorUnlocked(); // Avoid excessive election noise while in this state. return Status::IllegalState("Not starting election: Node is currently " "a non-participant in the raft config", @@ -697,10 +698,7 @@ Status RaftConsensus::Update(const ConsensusRequestPB* request, // Helper function to check if the op is a non-Transaction op. static bool IsConsensusOnlyOperation(OperationType op_type) { - if (op_type == NO_OP || op_type == CHANGE_CONFIG_OP) { - return true; - } - return false; + return op_type == NO_OP || op_type == CHANGE_CONFIG_OP; } Status RaftConsensus::StartReplicaTransactionUnlocked(const ReplicateRefPtr& msg) { @@ -829,9 +827,8 @@ Status RaftConsensus::HandleLeaderRequestTermUnlocked(const ConsensusRequestPB* ConsensusErrorPB::INVALID_TERM, Status::IllegalState(msg)); return Status::OK(); - } else { - RETURN_NOT_OK(HandleTermAdvanceUnlocked(request->caller_term())); } + RETURN_NOT_OK(HandleTermAdvanceUnlocked(request->caller_term())); } return Status::OK(); } @@ -1116,8 +1113,8 @@ Status RaftConsensus::UpdateReplica(const ConsensusRequestPB* request, Status prepare_status; auto iter = deduped_req.messages.begin(); - if (PREDICT_TRUE(deduped_req.messages.size() > 0)) { - // TODO Temporary until the leader explicitly propagates the safe timestamp. + if (PREDICT_TRUE(!deduped_req.messages.empty())) { + // TODO(KUDU-798) Temporary until the leader explicitly propagates the safe timestamp. clock_->Update(Timestamp(deduped_req.messages.back()->get()->timestamp())); // This request contains at least one message, and is likely to increase @@ -1184,7 +1181,7 @@ Status RaftConsensus::UpdateReplica(const ConsensusRequestPB* request, // 3 - Enqueue the writes. // Now that we've triggered the prepares enqueue the operations to be written // to the WAL. - if (PREDICT_TRUE(deduped_req.messages.size() > 0)) { + if (PREDICT_TRUE(!deduped_req.messages.empty())) { last_from_leader = deduped_req.messages.back()->get()->id(); // Trigger the log append asap, if fsync() is on this might take a while // and we can't reply until this is done. @@ -1237,7 +1234,7 @@ Status RaftConsensus::UpdateReplica(const ConsensusRequestPB* request, // We'll re-acquire it before we update the state again. // Update the last replicated op id - if (deduped_req.messages.size() > 0) { + if (!deduped_req.messages.empty()) { // 5 - We wait for the writes to be durable. @@ -1919,7 +1916,7 @@ void RaftConsensus::NonTxRoundReplicationFinished(ConsensusRound* round, const StatusCallback& client_cb, const Status& status) { OperationType op_type = round->replicate_msg()->op_type(); - string op_type_str = OperationType_Name(op_type); + const string& op_type_str = OperationType_Name(op_type); CHECK(IsConsensusOnlyOperation(op_type)) << "Unexpected op type: " << op_type_str; if (!status.ok()) { // In the case that a change-config operation is aborted, RaftConsensusState http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/raft_consensus.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h index 90b10b6..ec48edb 100644 --- a/src/kudu/consensus/raft_consensus.h +++ b/src/kudu/consensus/raft_consensus.h @@ -339,7 +339,7 @@ class RaftConsensus : public Consensus, VoteResponsePB* response); // Respond to VoteRequest that the candidate's last-logged OpId is too old. - Status RequestVoteRespondLastOpIdTooOld(const OpId& local_last_opid, + Status RequestVoteRespondLastOpIdTooOld(const OpId& local_last_logged_opid, const VoteRequestPB* request, VoteResponsePB* response); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/raft_consensus_quorum-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus_quorum-test.cc b/src/kudu/consensus/raft_consensus_quorum-test.cc index 648cb1d..04e894a 100644 --- a/src/kudu/consensus/raft_consensus_quorum-test.cc +++ b/src/kudu/consensus/raft_consensus_quorum-test.cc @@ -67,7 +67,6 @@ using log::Log; using log::LogEntryPB; using log::LogOptions; using log::LogReader; -using rpc::RpcContext; using strings::Substitute; using strings::SubstituteAndAppend; @@ -130,9 +129,9 @@ class RaftConsensusQuorumTest : public KuduTest { kTestTablet, schema_, 0, // schema_version - NULL, + nullptr, &log)); - logs_.push_back(log.get()); + logs_.emplace_back(std::move(log)); fs_managers_.push_back(fs_manager.release()); } return Status::OK(); @@ -326,7 +325,7 @@ class RaftConsensusQuorumTest : public KuduTest { if (MonoTime::Now() > (start + timeout)) { break; } - SleepFor(MonoDelta::FromMilliseconds(1 << backoff_exp)); + SleepFor(MonoDelta::FromMilliseconds(1LL << backoff_exp)); backoff_exp = std::min(backoff_exp + 1, kMaxBackoffExp); } @@ -855,7 +854,7 @@ TEST_F(RaftConsensusQuorumTest, TestLeaderHeartbeats) { // Now wait for about 4 times the hearbeat period the counters // should have increased 3/4 times. - SleepFor(MonoDelta::FromMilliseconds(FLAGS_raft_heartbeat_interval_ms * 4)); + SleepFor(MonoDelta::FromMilliseconds(FLAGS_raft_heartbeat_interval_ms * 4LL)); int repl0_final_count = counter_hook_rpl0->num_pre_update_calls(); int repl1_final_count = counter_hook_rpl1->num_pre_update_calls(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/raft_consensus_state-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus_state-test.cc b/src/kudu/consensus/raft_consensus_state-test.cc index a63231b..f5d4fa9 100644 --- a/src/kudu/consensus/raft_consensus_state-test.cc +++ b/src/kudu/consensus/raft_consensus_state-test.cc @@ -20,9 +20,9 @@ #include <memory> #include <vector> +#include "kudu/consensus/consensus-test-util.h" #include "kudu/consensus/consensus.pb.h" #include "kudu/consensus/consensus_meta.h" -#include "kudu/consensus/consensus-test-util.h" #include "kudu/fs/fs_manager.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" @@ -31,9 +31,8 @@ namespace kudu { namespace consensus { using std::unique_ptr; -using std::vector; -// TODO: Share a test harness with ConsensusMetadataTest? +// TODO(mpercy): Share a test harness with ConsensusMetadataTest? const char* kTabletId = "TestTablet"; class RaftConsensusStateTest : public KuduTest { http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/src/kudu/consensus/raft_consensus_state.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus_state.h b/src/kudu/consensus/raft_consensus_state.h index 174458f..e27b7b3 100644 --- a/src/kudu/consensus/raft_consensus_state.h +++ b/src/kudu/consensus/raft_consensus_state.h @@ -37,7 +37,6 @@ namespace kudu { class HostPort; -class ReplicaState; class ThreadPool; namespace rpc { @@ -100,7 +99,7 @@ class ReplicaState { std::unique_ptr<ConsensusMetadata> cmeta, ReplicaTransactionFactory* txn_factory); - Status StartUnlocked(const OpId& last_in_wal); + Status StartUnlocked(const OpId& last_id_in_wal); // Locks a replica in preparation for StartUnlocked(). Makes // sure the replica is in kInitialized state. @@ -185,7 +184,7 @@ class ReplicaState { // Changes the committed config for this replica. Checks that there is a // pending configuration and that it is equal to this one. Persists changes to disk. // Resets the pending configuration to null. - Status SetCommittedConfigUnlocked(const RaftConfigPB& new_config); + Status SetCommittedConfigUnlocked(const RaftConfigPB& committed_config); // Return the persisted configuration. const RaftConfigPB& GetCommittedConfigUnlocked() const; http://git-wip-us.apache.org/repos/asf/kudu/blob/c5b07fa8/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 83d7869..86b688b 100644 --- a/src/kudu/tablet/tablet_bootstrap-test.cc +++ b/src/kudu/tablet/tablet_bootstrap-test.cc @@ -38,14 +38,6 @@ using std::unique_ptr; using std::vector; namespace kudu { - -namespace log { - -extern const char* kTestTable; -extern const char* kTestTablet; - -} // namespace log - namespace tablet { using consensus::ConsensusBootstrapInfo;
