[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 <aser...@cloudera.com>
Reviewed-by: Mike Percy <mpe...@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 <aser...@cloudera.com>
Authored: Fri Apr 20 18:37:25 2018 -0700
Committer: Alexey Serbin <aser...@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();
