kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [kudu] 01/07: [consensus] KUDU-2839 fix assert in GetParticipantRole()
Date Sun, 29 Dec 2019 18:58:42 GMT
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 <alexey@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 <bankim@cloudera.com>
    Reviewed-by: Adar Dembo <adar@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.


Mime
View raw message