You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2019/12/29 18:58:42 UTC
[kudu] 01/07: [consensus] KUDU-2839 fix assert in
GetParticipantRole()
This is an automated email from the ASF dual-hosted git repository.
adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit a195d0fd84389437640ca356a30b6679d4aae424
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Mon Dec 23 20:40:51 2019 -0800
[consensus] KUDU-2839 fix assert in GetParticipantRole()
Before this patch, the consistency check in GetParticipantRole() was
incorrect. It tried to search for the replica UUID against pending
config, but replica UUID comes from committed config at the only call
site of the function in CatalogManager::BuildLocationsForTablet().
The pending Raft config might have replica already removed in case if
there is move/replace replica operation in progress. The incorrect
assertion triggered here:
http://dist-test.cloudera.org/job?job_id=aserbin.1577145505.5646
I also did a minor clean up of the code around.
This is a follow-up to 586e957f76a547340f2ab93a7eebc3f116ff825e.
Change-Id: I1a490f5b61dda8cebb8ee9bd9171ef3aa733a148
Reviewed-on: http://gerrit.cloudera.org:8080/14946
Tested-by: Kudu Jenkins
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
src/kudu/consensus/quorum_util.cc | 10 ++++++----
src/kudu/consensus/quorum_util.h | 4 +---
src/kudu/master/catalog_manager.cc | 11 +++++------
src/kudu/master/catalog_manager.h | 1 -
4 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc
index dccf0fa..0b7300e 100644
--- a/src/kudu/consensus/quorum_util.cc
+++ b/src/kudu/consensus/quorum_util.cc
@@ -161,14 +161,16 @@ RaftPeerPB::Role GetConsensusRole(const std::string& peer_uuid,
return GetConsensusRole(peer_uuid, cstate.leader_uuid(), config);
}
-
RaftPeerPB::Role GetParticipantRole(const RaftPeerPB& peer,
const ConsensusStatePB& cstate) {
const auto& peer_uuid = peer.permanent_uuid();
+ // Sanity check: make sure the peer is in the committed config.
DCHECK_NE(RaftPeerPB::NON_PARTICIPANT,
- GetConsensusRole(peer_uuid, cstate))
- << "Peer " << peer_uuid << " << not a participant in " << cstate.ShortDebugString();
-
+ GetConsensusRole(peer_uuid,
+ cstate.leader_uuid(),
+ cstate.committed_config()))
+ << Substitute("peer $0 is not commited config $1",
+ peer_uuid, cstate.ShortDebugString());
switch (peer.member_type()) {
case RaftPeerPB::VOTER:
if (peer_uuid == cstate.leader_uuid()) {
diff --git a/src/kudu/consensus/quorum_util.h b/src/kudu/consensus/quorum_util.h
index e6afbaa..b78c910 100644
--- a/src/kudu/consensus/quorum_util.h
+++ b/src/kudu/consensus/quorum_util.h
@@ -81,14 +81,12 @@ RaftPeerPB::Role GetConsensusRole(const std::string& peer_uuid,
RaftPeerPB::Role GetConsensusRole(const std::string& peer_uuid,
const ConsensusStatePB& cstate);
-
// Same as above, but requires that the given 'peer' is a participant
-// in the active configuration in specified consensus state.
+// in the committed configuration in specified consensus state.
// If not, it will return incorrect results.
RaftPeerPB::Role GetParticipantRole(const RaftPeerPB& peer,
const ConsensusStatePB& cstate);
-
// Verifies that the provided configuration is well formed.
Status VerifyRaftConfig(const RaftConfigPB& config);
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 05ef2a7..027ef6c 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -4827,7 +4827,7 @@ Status CatalogManager::BuildLocationsForTablet(
// Helper function to create a TSInfoPB.
auto make_tsinfo_pb = [this, &peer]() {
- unique_ptr<TSInfoPB> tsinfo_pb(new TSInfoPB());
+ unique_ptr<TSInfoPB> tsinfo_pb(new TSInfoPB);
tsinfo_pb->set_permanent_uuid(peer.permanent_uuid());
shared_ptr<TSDescriptor> ts_desc;
if (master_->ts_manager()->LookupTSByUUID(peer.permanent_uuid(), &ts_desc)) {
@@ -4845,19 +4845,18 @@ Status CatalogManager::BuildLocationsForTablet(
return tsinfo_pb;
};
- auto role = GetParticipantRole(peer, cstate);
+ const auto role = GetParticipantRole(peer, cstate);
optional<string> dimension = none;
if (l_tablet.data().pb.has_dimension_label()) {
dimension = l_tablet.data().pb.dimension_label();
}
if (ts_infos_dict) {
- int idx = *ComputePairIfAbsent(
+ const auto idx = *ComputePairIfAbsent(
&ts_infos_dict->uuid_to_idx, peer.permanent_uuid(),
[&]() -> pair<StringPiece, int> {
auto& ts_info_pbs = ts_infos_dict->ts_info_pbs;
- auto pb = make_tsinfo_pb();
- int ts_info_idx = ts_info_pbs.size();
- ts_info_pbs.emplace_back(pb.release());
+ auto ts_info_idx = ts_info_pbs.size();
+ ts_info_pbs.emplace_back(make_tsinfo_pb().release());
return { ts_info_pbs.back()->permanent_uuid(), ts_info_idx };
});
diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h
index 0363a36..2d3f3a9 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -913,7 +913,6 @@ class CatalogManager : public tserver::TabletReplicaLookupIf {
const PartitionPB& partition,
const boost::optional<std::string>& dimension_label);
-
// Builds the TabletLocationsPB for a tablet based on the provided TabletInfo
// and the replica type fiter specified. Populates locs_pb and returns
// Status::OK on success.