kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject [2/2] kudu git commit: KUDU-564 (part 1): log a 'diff' when tablet config changes
Date Fri, 12 Aug 2016 01:59:40 GMT
KUDU-564 (part 1): log a 'diff' when tablet config changes

This adds some code to 'diff' two consensus configurations, so that log
messages in the master and during config change are much easier to read.
For example, after the update, raft_consensus-itest has logs like:

I0810 16:05:15.715248 21161 raft_consensus_state.cc:609] T 382e9565bbc64aefb973d1f0b21fcc7c
P d4ce96ae644c45e1a867f4e14e6f6746 [term 1 NON_PARTICIPANT]: Committing config change with
OpId 1.2: config changed from index -1 to 2, VOTER P d4ce96ae644c45e1a867f4e14e6f6746 (127.37.24.4)
evicted. New config: { opid_index: 2 peers { permanent_uuid: "544750e465814b6087b2c44a23d240f4"
member_type: VOTER last_known_addr { host: "127.37.24.0" port: 39133 } } peers { permanent_uuid:
"535909db4a48478288433b7f6999565e" member_type: VOTER last_known_addr { host: "127.37.24.3"
port: 35244 } } peers { permanent_uuid: "9b9c692a90854ba7909a47912fc3d0bb" member_type: VOTER
last_known_addr { host: "127.37.24.1" port: 39890 } } peers { permanent_uuid: "13754a25859d4fd1a97fcaa0fcb6f849"
member_type: VOTER last_known_addr { host: "127.37.24.2" port: 40505 } } }

I left in the dumping of the full 'new config' since having all the
information can still be relevant, but the human readable summary makes
this much easier to interpret.

Change-Id: Icb785093176eea067de039b14f6600084998861a
Reviewed-on: http://gerrit.cloudera.org:8080/3939
Tested-by: Kudu Jenkins
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/564eb4ed
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/564eb4ed
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/564eb4ed

Branch: refs/heads/master
Commit: 564eb4ed1440aeadc0c3948168be8fe12888a46d
Parents: 1a58996
Author: Todd Lipcon <todd@apache.org>
Authored: Wed Aug 10 16:07:56 2016 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Fri Aug 12 01:59:06 2016 +0000

----------------------------------------------------------------------
 src/kudu/consensus/quorum_util-test.cc     |  72 +++++++++++++++
 src/kudu/consensus/quorum_util.cc          | 117 +++++++++++++++++++++++-
 src/kudu/consensus/quorum_util.h           |  11 ++-
 src/kudu/consensus/raft_consensus_state.cc |   6 +-
 src/kudu/master/catalog_manager.cc         |   5 +-
 5 files changed, 204 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/564eb4ed/src/kudu/consensus/quorum_util-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util-test.cc b/src/kudu/consensus/quorum_util-test.cc
index 8d99cf0..4b9f50d 100644
--- a/src/kudu/consensus/quorum_util-test.cc
+++ b/src/kudu/consensus/quorum_util-test.cc
@@ -31,6 +31,7 @@ static void SetPeerInfo(const string& uuid,
                         RaftPeerPB* peer) {
   peer->set_permanent_uuid(uuid);
   peer->set_member_type(type);
+  peer->mutable_last_known_addr()->set_host(uuid + ".example.com");
 }
 
 TEST(QuorumUtilTest, TestMemberExtraction) {
@@ -56,5 +57,76 @@ TEST(QuorumUtilTest, TestMemberExtraction) {
   ASSERT_EQ("B", peer_pb.permanent_uuid());
 }
 
+TEST(QuorumUtilTest, TestDiffConsensusStates) {
+  ConsensusStatePB old_cs;
+  SetPeerInfo("A", RaftPeerPB::VOTER, old_cs.mutable_config()->add_peers());
+  SetPeerInfo("B", RaftPeerPB::VOTER, old_cs.mutable_config()->add_peers());
+  SetPeerInfo("C", RaftPeerPB::VOTER, old_cs.mutable_config()->add_peers());
+  old_cs.set_current_term(1);
+  old_cs.set_leader_uuid("A");
+  old_cs.mutable_config()->set_opid_index(1);
+
+  // Simple case of no change.
+  EXPECT_EQ("no change",
+            DiffConsensusStates(old_cs, old_cs));
+
+  // Simulate a leader change.
+  {
+    auto new_cs = old_cs;
+    new_cs.set_leader_uuid("B");
+    new_cs.set_current_term(2);
+
+    EXPECT_EQ("term changed from 1 to 2, "
+              "leader changed from A (A.example.com) to B (B.example.com)",
+              DiffConsensusStates(old_cs, new_cs));
+  }
+
+  // Simulate eviction of a peer.
+  {
+    auto new_cs = old_cs;
+    new_cs.mutable_config()->set_opid_index(2);
+    new_cs.mutable_config()->mutable_peers()->RemoveLast();
+
+    EXPECT_EQ("config changed from index 1 to 2, "
+              "VOTER C (C.example.com) evicted",
+              DiffConsensusStates(old_cs, new_cs));
+  }
+
+  // Simulate addition of a peer.
+  {
+    auto new_cs = old_cs;
+    new_cs.mutable_config()->set_opid_index(2);
+    SetPeerInfo("D", RaftPeerPB::NON_VOTER, new_cs.mutable_config()->add_peers());
+
+    EXPECT_EQ("config changed from index 1 to 2, "
+              "NON_VOTER D (D.example.com) added",
+              DiffConsensusStates(old_cs, new_cs));
+  }
+
+  // Simulate change of a peer's member type.
+  {
+    auto new_cs = old_cs;
+    new_cs.mutable_config()->set_opid_index(2);
+    new_cs.mutable_config()->mutable_peers()->Mutable(2)->set_member_type(RaftPeerPB::NON_VOTER);
+
+    EXPECT_EQ("config changed from index 1 to 2, "
+              "C (C.example.com) changed from VOTER to NON_VOTER",
+              DiffConsensusStates(old_cs, new_cs));
+  }
+
+  // Simulate change from no leader to a leader
+  {
+    auto no_leader_cs = old_cs;
+    no_leader_cs.clear_leader_uuid();
+    auto new_cs = old_cs;
+    new_cs.set_current_term(2);
+
+    EXPECT_EQ("term changed from 1 to 2, "
+              "leader changed from <none> to A (A.example.com)",
+              DiffConsensusStates(no_leader_cs, new_cs));
+  }
+
+}
+
 } // namespace consensus
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/564eb4ed/src/kudu/consensus/quorum_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc
index 26cc4a6..44dc306 100644
--- a/src/kudu/consensus/quorum_util.cc
+++ b/src/kudu/consensus/quorum_util.cc
@@ -16,10 +16,14 @@
 // under the License.
 #include "kudu/consensus/quorum_util.h"
 
+#include <map>
 #include <set>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "kudu/gutil/map-util.h"
+#include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/util/status.h"
 
@@ -27,6 +31,8 @@ namespace kudu {
 namespace consensus {
 
 using google::protobuf::RepeatedPtrField;
+using std::map;
+using std::pair;
 using std::string;
 using strings::Substitute;
 
@@ -196,5 +202,114 @@ Status VerifyConsensusState(const ConsensusStatePB& cstate, RaftConfigState
type
   return Status::OK();
 }
 
-} // namespace consensus
+std::string DiffRaftConfigs(const RaftConfigPB& old_config,
+                            const RaftConfigPB& new_config) {
+  // Create dummy ConsensusState objects so we can reuse the code
+  // from the below function.
+  ConsensusStatePB old_state;
+  old_state.mutable_config()->CopyFrom(old_config);
+  ConsensusStatePB new_state;
+  new_state.mutable_config()->CopyFrom(new_config);
+
+  return DiffConsensusStates(old_state, new_state);
+}
+
+string DiffConsensusStates(const ConsensusStatePB& old_state,
+                           const ConsensusStatePB& new_state) {
+  bool leader_changed = old_state.leader_uuid() != new_state.leader_uuid();
+  bool term_changed = old_state.current_term() != new_state.current_term();
+  bool config_changed = old_state.config().opid_index() != new_state.config().opid_index();
+
+  // Construct a map from Peer UUID to '<old peer, new peer>' pairs.
+  // Due to the default construction nature of std::map and std::pair, if a peer
+  // is present in one configuration but not the other, we'll end up with an empty
+  // protobuf in that element of the pair.
+  map<string, pair<RaftPeerPB, RaftPeerPB>> peer_infos;
+  for (const auto& p : old_state.config().peers()) {
+    peer_infos[p.permanent_uuid()].first = p;
+  }
+  for (const auto& p : new_state.config().peers()) {
+    peer_infos[p.permanent_uuid()].second = p;
+  }
+
+  // Now collect strings representing the changes.
+  vector<string> change_strs;
+  if (config_changed) {
+    change_strs.push_back(
+        Substitute("config changed from index $0 to $1",
+                   old_state.config().opid_index(),
+                   new_state.config().opid_index()));
+  }
+
+  if (term_changed) {
+    change_strs.push_back(
+        Substitute("term changed from $0 to $1",
+                   old_state.current_term(),
+                   new_state.current_term()));
+  }
+
+  if (leader_changed) {
+    string old_leader = "<none>";
+    string new_leader = "<none>";
+    if (old_state.has_leader_uuid()) {
+      old_leader = Substitute("$0 ($1)",
+                              old_state.leader_uuid(),
+                              peer_infos[old_state.leader_uuid()].first.last_known_addr().host());
+    }
+    if (new_state.has_leader_uuid()) {
+      new_leader = Substitute("$0 ($1)",
+                              new_state.leader_uuid(),
+                              peer_infos[new_state.leader_uuid()].second.last_known_addr().host());
+    }
+
+    change_strs.push_back(Substitute("leader changed from $0 to $1",
+                                     old_leader, new_leader));
+  }
+
+  for (const auto& e : peer_infos) {
+    const auto& old_peer = e.second.first;
+    const auto& new_peer = e.second.second;
+    if (old_peer.has_permanent_uuid() && !new_peer.has_permanent_uuid()) {
+      change_strs.push_back(
+          Substitute("$0 $1 ($2) evicted",
+                     RaftPeerPB_MemberType_Name(old_peer.member_type()),
+                     old_peer.permanent_uuid(),
+                     old_peer.last_known_addr().host()));
+    } else if (!old_peer.has_permanent_uuid() && new_peer.has_permanent_uuid()) {
+      change_strs.push_back(
+          Substitute("$0 $1 ($2) added",
+                     RaftPeerPB_MemberType_Name(new_peer.member_type()),
+                     new_peer.permanent_uuid(),
+                     new_peer.last_known_addr().host()));
+    } else if (old_peer.has_permanent_uuid() && new_peer.has_permanent_uuid()) {
+      if (old_peer.member_type() != new_peer.member_type()) {
+        change_strs.push_back(
+            Substitute("$0 ($1) changed from $2 to $3",
+                       old_peer.permanent_uuid(),
+                       old_peer.last_known_addr().host(),
+                       RaftPeerPB_MemberType_Name(old_peer.member_type()),
+                       RaftPeerPB_MemberType_Name(new_peer.member_type())));
+      }
+    }
+  }
+
+  // We expect to have detected some differences above, but in case
+  // someone forgets to update this function when adding a new field,
+  // it's still useful to report some change unless the protobufs are identical.
+  // So, we fall back to just dumping the before/after debug strings.
+  if (change_strs.empty()) {
+    if (old_state.ShortDebugString() == new_state.ShortDebugString()) {
+      return "no change";
+    }
+    return Substitute("change from {$0} to {$1}",
+                      old_state.ShortDebugString(),
+                      new_state.ShortDebugString());
+  }
+
+
+  return JoinStrings(change_strs, ", ");
+
+}
+
+}  // namespace consensus
 }  // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/564eb4ed/src/kudu/consensus/quorum_util.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.h b/src/kudu/consensus/quorum_util.h
index 3d1ccb0..ed21e9d 100644
--- a/src/kudu/consensus/quorum_util.h
+++ b/src/kudu/consensus/quorum_util.h
@@ -62,7 +62,7 @@ int MajoritySize(int num_voters);
 // If the peer uuid is not a voter in the configuration, this function will return
 // NON_PARTICIPANT, regardless of whether it is listed as leader in cstate.
 RaftPeerPB::Role GetConsensusRole(const std::string& uuid,
-                                    const ConsensusStatePB& cstate);
+                                  const ConsensusStatePB& cstate);
 
 // Verifies that the provided configuration is well formed.
 // If type == COMMITTED_QUORUM, we enforce that opid_index is set.
@@ -73,6 +73,15 @@ Status VerifyRaftConfig(const RaftConfigPB& config, RaftConfigState
type);
 // leader is a configuration voter, if it is set, and that a valid term is set.
 Status VerifyConsensusState(const ConsensusStatePB& cstate, RaftConfigState type);
 
+// Provide a textual description of the difference between two consensus states,
+// suitable for logging.
+std::string DiffConsensusStates(const ConsensusStatePB& old_state,
+                                const ConsensusStatePB& new_state);
+
+// Same as the above, but just the RaftConfigPB portion of the configuration.
+std::string DiffRaftConfigs(const RaftConfigPB& old_config,
+                            const RaftConfigPB& new_config);
+
 }  // namespace consensus
 }  // namespace kudu
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/564eb4ed/src/kudu/consensus/raft_consensus_state.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus_state.cc b/src/kudu/consensus/raft_consensus_state.cc
index 3b3e703..7d937df 100644
--- a/src/kudu/consensus/raft_consensus_state.cc
+++ b/src/kudu/consensus/raft_consensus_state.cc
@@ -607,9 +607,9 @@ Status ReplicaState::AdvanceCommittedIndexUnlocked(const OpId& committed_index,
       const RaftConfigPB& committed_config = GetCommittedConfigUnlocked();
       if (new_config.opid_index() > committed_config.opid_index()) {
         LOG_WITH_PREFIX_UNLOCKED(INFO) << "Committing config change with OpId "
-            << current_id << ". "
-            << "Old config: { " << old_config.ShortDebugString() << " }.
"
-            << "New config: { " << new_config.ShortDebugString() << " }";
+            << current_id << ": "
+            << DiffRaftConfigs(old_config, new_config)
+            << ". New config: { " << new_config.ShortDebugString() << "
}";
         CHECK_OK(SetCommittedConfigUnlocked(new_config));
       } else {
         LOG_WITH_PREFIX_UNLOCKED(INFO) << "Ignoring commit of config change with OpId
"

http://git-wip-us.apache.org/repos/asf/kudu/blob/564eb4ed/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index e2061d9..49001f8 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -1979,8 +1979,9 @@ Status CatalogManager::HandleReportedTablet(TSDescriptor* ts_desc,
 
       // If a replica is reporting a new consensus configuration, update the
       // master's copy of that configuration.
-      LOG(INFO) << "Tablet: " << tablet->tablet_id() << " reported consensus
state change."
-                << " New consensus state: " << cstate.ShortDebugString();
+      LOG(INFO) << "T " << tablet->tablet_id() << " reported consensus
state change: "
+                << DiffConsensusStates(prev_cstate, cstate)
+                << ". New consensus state: " << cstate.ShortDebugString();
 
       // If we need to change the report, copy the whole thing on the stack
       // rather than const-casting.


Mime
View raw message