kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject [2/3] kudu git commit: [consensus] update UnsafeChangeConfig() signature
Date Fri, 22 Jun 2018 22:28:19 GMT
[consensus] update UnsafeChangeConfig() signature

While testing various scenarios involving kudu CLI's
'remote_replica unsafe_change_config' sub-command, it turned out that
tserver might end up trying to respond with invalid value of
TabletServerErrorPB::Code.  In that case, a tserver built in DEBUG
configuration would crash with the following stack trace:

      ../nptl/sysdeps/unix/sysv/linux/raise.c:64
    assertion=0x7fb4eab7c4f8
      "::kudu::tserver::TabletServerErrorPB_Code_IsValid(value)",
    file=0x7fb4eab7c531 "src/kudu/tserver/tserver.pb.h",
    line=<value optimized out>, function=<value optimized out>) at assert.c:96
    assertion=0x7fb4eab7c4f8
      "::kudu::tserver::TabletServerErrorPB_Code_IsValid(value)",
    file=0x7fb4eab7c531 "src/kudu/tserver/tserver.pb.h", line=2623,
    function=0x7fb4eab7c586 "void kudu::tserver::TabletServerErrorPB::set_code(
      ::kudu::tserver::TabletServerErrorPB_Code)") at assert.c:105
      kudu::tserver::TabletServerErrorPB_Code) ()
    at src/kudu/tserver/tserver.pb.h:2623
      kudu::tserver::TabletServerErrorPB*,
      kudu::Status const&, kudu::tserver::TabletServerErrorPB_Code,
      kudu::rpc::RpcContext*) ()
    at src/kudu/tserver/tablet_service.cc:370
      kudu::consensus::UnsafeChangeConfigRequestPB const*,
      kudu::consensus::UnsafeChangeConfigResponsePB*,
      kudu::rpc::RpcContext*) () at src/kudu/tserver/tablet_service.cc:1060

This changelist fixes the issue, 'unifying' the signatures of the
RaftConsensus::{ChangeConfig,BulkChangeConfig,UpdateChangeConfig}()
methods.

Change-Id: Id266dd72ef5e44e9b4e2ce9df10694bce8cfe2fb
Reviewed-on: http://gerrit.cloudera.org:8080/10141
Tested-by: Alexey Serbin <aserbin@cloudera.com>
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/f1e652b2
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f1e652b2
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f1e652b2

Branch: refs/heads/master
Commit: f1e652b23c2bd76033868fe6a30484cb30098649
Parents: 009cfe2
Author: Alexey Serbin <aserbin@cloudera.com>
Authored: Fri Apr 20 18:37:25 2018 -0700
Committer: Alexey Serbin <aserbin@cloudera.com>
Committed: Fri Jun 22 18:05:06 2018 +0000

----------------------------------------------------------------------
 src/kudu/consensus/raft_consensus.cc | 22 ++++++++++------------
 src/kudu/consensus/raft_consensus.h  |  2 +-
 src/kudu/tserver/tablet_service.cc   | 10 ++++++----
 3 files changed, 17 insertions(+), 17 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/f1e652b2/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index 68f1859..b4ef789 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -1886,8 +1886,9 @@ Status RaftConsensus::BulkChangeConfig(const BulkChangeConfigRequestPB&
req,
   return Status::OK();
 }
 
-Status RaftConsensus::UnsafeChangeConfig(const UnsafeChangeConfigRequestPB& req,
-                                         TabletServerErrorPB::Code* error_code) {
+Status RaftConsensus::UnsafeChangeConfig(
+    const UnsafeChangeConfigRequestPB& req,
+    boost::optional<tserver::TabletServerErrorPB::Code>* error_code) {
   if (PREDICT_FALSE(!req.has_new_config())) {
     *error_code = TabletServerErrorPB::INVALID_CONFIG;
     return Status::InvalidArgument("Request must contain 'new_config' argument "
@@ -1960,6 +1961,7 @@ Status RaftConsensus::UnsafeChangeConfig(const UnsafeChangeConfigRequestPB&
req,
   // in the committed config, it is rare and a replica without itself
   // in the latest config is definitely not caught up with the latest leader's log.
   if (!IsRaftConfigVoter(peer_uuid(), new_config)) {
+    *error_code = TabletServerErrorPB::INVALID_CONFIG;
     return Status::InvalidArgument(Substitute("Local replica uuid $0 is not "
                                               "a VOTER in the new config, "
                                               "rejecting the unsafe config "
@@ -1985,13 +1987,12 @@ Status RaftConsensus::UnsafeChangeConfig(const UnsafeChangeConfigRequestPB&
req,
   // Prepare the consensus request as if the request is being generated
   // from a different leader.
   ConsensusRequestPB consensus_req;
-  ConsensusResponsePB consensus_resp;
   consensus_req.set_caller_uuid(req.caller_id());
   // Bumping up the term for the consensus request being generated.
   // This makes this request appear to come from a new leader that
   // the local replica doesn't know about yet. If the local replica
   // happens to be the leader, this will cause it to step down.
-  int64_t new_term = current_term + 1;
+  const int64_t new_term = current_term + 1;
   consensus_req.set_caller_term(new_term);
   consensus_req.mutable_preceding_id()->CopyFrom(preceding_opid);
   consensus_req.set_committed_index(last_committed_index);
@@ -2018,14 +2019,11 @@ Status RaftConsensus::UnsafeChangeConfig(const UnsafeChangeConfigRequestPB&
req,
         << "COMMITTED CONFIG: " << SecureShortDebugString(committed_config)
         << "NEW CONFIG: " << SecureShortDebugString(new_config);
 
-  s = Update(&consensus_req, &consensus_resp);
-  if (!s.ok() || consensus_resp.has_error()) {
-    *error_code = TabletServerErrorPB::UNKNOWN_ERROR;
-  }
-  if (s.ok() && consensus_resp.has_error()) {
-    s = StatusFromPB(consensus_resp.error().status());
-  }
-  return s;
+  ConsensusResponsePB consensus_resp;
+  return Update(&consensus_req, &consensus_resp).AndThen([&consensus_resp]{
+    return consensus_resp.has_error()
+        ? StatusFromPB(consensus_resp.error().status()) : Status::OK();
+  });
 }
 
 void RaftConsensus::Stop() {

http://git-wip-us.apache.org/repos/asf/kudu/blob/f1e652b2/src/kudu/consensus/raft_consensus.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h
index 33e8196..f84a71e 100644
--- a/src/kudu/consensus/raft_consensus.h
+++ b/src/kudu/consensus/raft_consensus.h
@@ -274,7 +274,7 @@ class RaftConsensus : public std::enable_shared_from_this<RaftConsensus>,
 
   // Implement an UnsafeChangeConfig() request.
   Status UnsafeChangeConfig(const UnsafeChangeConfigRequestPB& req,
-                            tserver::TabletServerErrorPB::Code* error_code);
+                            boost::optional<tserver::TabletServerErrorPB::Code>* error_code);
 
   // Returns the last OpId (either received or committed, depending on the
   // 'type' argument) that the Consensus implementation knows about.

http://git-wip-us.apache.org/repos/asf/kudu/blob/f1e652b2/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index 89dbf95..4cf88ea 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -1086,11 +1086,13 @@ void ConsensusServiceImpl::UnsafeChangeConfig(const UnsafeChangeConfigRequestPB*
   }
 
   shared_ptr<RaftConsensus> consensus;
-  if (!GetConsensusOrRespond(replica, resp, context, &consensus)) return;
-  TabletServerErrorPB::Code error_code;
-  Status s = consensus->UnsafeChangeConfig(*req, &error_code);
+  if (!GetConsensusOrRespond(replica, resp, context, &consensus)) {
+    return;
+  }
+  boost::optional<TabletServerErrorPB::Code> error_code;
+  const Status s = consensus->UnsafeChangeConfig(*req, &error_code);
   if (PREDICT_FALSE(!s.ok())) {
-    SetupErrorAndRespond(resp->mutable_error(), s, error_code, context);
+    HandleErrorResponse(req, resp, context, error_code, s);
     return;
   }
   context->RespondSuccess();


Mime
View raw message