You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2018/03/09 22:38:19 UTC
[1/3] kudu git commit: KUDU-2274. RaftConsensus should not access
cmeta when shutdown
Repository: kudu
Updated Branches:
refs/heads/branch-1.6.x e8c68e6c0 -> 3aa933191
KUDU-2274. RaftConsensus should not access cmeta when shutdown
An additional case of KUDU-2274 found during stress testing was that
RaftConsensus will return a ConsensusStatePB from the ConsensusState()
RPC method even when shutdown. This patch prevents that.
Conflicts:
src/kudu/tablet/tablet_replica.h
src/kudu/tserver/tserver_path_handlers.cc
Change-Id: Ib3f3f75674d5739b281e7c689ddb2d433b9b7415
Reviewed-on: http://gerrit.cloudera.org:8080/9266
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
(cherry picked from commit d977d1cf16d5b8e82d84396a0a82351582258fa1)
Reviewed-on: http://gerrit.cloudera.org:8080/9548
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e605c162
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e605c162
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e605c162
Branch: refs/heads/branch-1.6.x
Commit: e605c16235c04a7aaeb0432ab0487ab3d1203728
Parents: e8c68e6
Author: Mike Percy <mp...@apache.org>
Authored: Thu Feb 8 21:34:08 2018 -0800
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Fri Mar 9 20:05:57 2018 +0000
----------------------------------------------------------------------
src/kudu/consensus/raft_consensus.cc | 13 ++++++----
src/kudu/consensus/raft_consensus.h | 5 +++-
.../ts_tablet_manager-itest.cc | 9 +++++--
src/kudu/master/catalog_manager.cc | 25 +++++++++++++-------
src/kudu/master/sys_catalog.cc | 8 ++++++-
src/kudu/tablet/tablet_replica.h | 5 ++++
src/kudu/tserver/tablet_copy_client-test.cc | 3 ++-
src/kudu/tserver/tablet_copy_source_session.cc | 4 ++--
src/kudu/tserver/tablet_server-test.cc | 20 ++++++++++++++++
src/kudu/tserver/tablet_service.cc | 12 ++++++----
src/kudu/tserver/ts_tablet_manager.cc | 7 +++++-
src/kudu/tserver/tserver_path_handlers.cc | 15 +++++++++++-
12 files changed, 100 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index cb3118e..3def94b 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -2326,10 +2326,14 @@ const string& RaftConsensus::tablet_id() const {
return options_.tablet_id;
}
-ConsensusStatePB RaftConsensus::ConsensusState(IncludeHealthReport report_health) const {
+Status RaftConsensus::ConsensusState(ConsensusStatePB* cstate,
+ IncludeHealthReport report_health) const {
ThreadRestrictions::AssertWaitAllowed();
UniqueLock l(lock_);
- ConsensusStatePB cstate = cmeta_->ToConsensusStatePB();
+ if (state_ == kShutdown) {
+ return Status::IllegalState("Tablet replica is shutdown");
+ }
+ ConsensusStatePB cstate_tmp = cmeta_->ToConsensusStatePB();
// If we need to include the health report, merge it into the committed
// config iff we believe we are the current leader of the config.
@@ -2342,7 +2346,7 @@ ConsensusStatePB RaftConsensus::ConsensusState(IncludeHealthReport report_health
// Iterate through each peer in the committed config and attach the health
// report to it.
- RaftConfigPB* committed_raft_config = cstate.mutable_committed_config();
+ RaftConfigPB* committed_raft_config = cstate_tmp.mutable_committed_config();
for (int i = 0; i < committed_raft_config->peers_size(); i++) {
RaftPeerPB* peer = committed_raft_config->mutable_peers(i);
const HealthReportPB* report = FindOrNull(reports, peer->permanent_uuid());
@@ -2350,7 +2354,8 @@ ConsensusStatePB RaftConsensus::ConsensusState(IncludeHealthReport report_health
*peer->mutable_health_report() = *report;
}
}
- return cstate;
+ *cstate = std::move(cstate_tmp);
+ return Status::OK();
}
RaftConfigPB RaftConsensus::CommittedConfig() const {
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/consensus/raft_consensus.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h
index 5ace791..bf44e83 100644
--- a/src/kudu/consensus/raft_consensus.h
+++ b/src/kudu/consensus/raft_consensus.h
@@ -296,7 +296,10 @@ class RaftConsensus : public std::enable_shared_from_this<RaftConsensus>,
// If 'report_health' is set to 'INCLUDE_HEALTH_REPORT', and if the
// local replica believes it is the leader of the config, it will include a
// health report about each active peer in the committed config.
- ConsensusStatePB ConsensusState(IncludeHealthReport report_health = EXCLUDE_HEALTH_REPORT) const;
+ // If RaftConsensus has been shut down, returns Status::IllegalState.
+ // Does not modify the out-param 'cstate' unless an OK status is returned.
+ Status ConsensusState(ConsensusStatePB* cstate,
+ IncludeHealthReport report_health = EXCLUDE_HEALTH_REPORT) const;
// Returns a copy of the current committed Raft configuration.
RaftConfigPB CommittedConfig() const;
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/integration-tests/ts_tablet_manager-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/ts_tablet_manager-itest.cc b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
index ee5a957..59c85a0 100644
--- a/src/kudu/integration-tests/ts_tablet_manager-itest.cc
+++ b/src/kudu/integration-tests/ts_tablet_manager-itest.cc
@@ -375,11 +375,16 @@ TEST_F(TsTabletManagerITest, ReportOnReplicaHealthStatus) {
string leader_uuid;
for (const auto& replica : tablet_replicas) {
RaftConsensus* consensus = CHECK_NOTNULL(replica->consensus());
- ConsensusStatePB cs(consensus->ConsensusState(RaftConsensus::INCLUDE_HEALTH_REPORT));
+ ConsensusStatePB cs;
+ Status s = consensus->ConsensusState(&cs, RaftConsensus::INCLUDE_HEALTH_REPORT);
+ if (!s.ok()) {
+ ASSERT_TRUE(s.IsIllegalState()) << s.ToString(); // Replica is shut down.
+ continue;
+ }
if (consensus->peer_uuid() == cs.leader_uuid()) {
// Only the leader replica has the up-to-date health report.
leader_uuid = cs.leader_uuid();
- cstate.Swap(&cs);
+ cstate = std::move(cs);
break;
}
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index acea922..b2b7c4e 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -675,7 +675,8 @@ Status CatalogManager::ElectedAsLeaderCb() {
}
Status CatalogManager::WaitUntilCaughtUpAsLeader(const MonoDelta& timeout) {
- ConsensusStatePB cstate = sys_catalog_->tablet_replica()->consensus()->ConsensusState();
+ ConsensusStatePB cstate;
+ RETURN_NOT_OK(sys_catalog_->tablet_replica()->consensus()->ConsensusState(&cstate));
const string& uuid = master_->fs_manager()->uuid();
if (cstate.leader_uuid() != uuid) {
return Status::IllegalState(
@@ -858,7 +859,7 @@ void CatalogManager::PrepareForLeadershipTask() {
shared_lock<LockType> l(lock_);
}
const RaftConsensus* consensus = sys_catalog_->tablet_replica()->consensus();
- const int64_t term_before_wait = consensus->ConsensusState().current_term();
+ const int64_t term_before_wait = consensus->CurrentTerm();
{
std::lock_guard<simple_spinlock> l(state_lock_);
if (leader_ready_term_ == term_before_wait) {
@@ -883,7 +884,7 @@ void CatalogManager::PrepareForLeadershipTask() {
return;
}
- const int64_t term = consensus->ConsensusState().current_term();
+ const int64_t term = consensus->CurrentTerm();
if (term_before_wait != term) {
// If we got elected leader again while waiting to catch up then we will
// get another callback to visit the tables and tablets, so bail.
@@ -921,7 +922,7 @@ void CatalogManager::PrepareForLeadershipTask() {
}
}
- const int64_t term = consensus.ConsensusState().current_term();
+ const int64_t term = consensus.CurrentTerm();
if (term != start_term) {
// If the term has changed we assume the new leader catalog is about
// to do the necessary work in its leadership preparation task.
@@ -4283,11 +4284,18 @@ CatalogManager::ScopedLeaderSharedLock::ScopedLeaderSharedLock(
catalog_->state_));
return;
}
+
+ ConsensusStatePB cstate;
+ Status s = catalog_->sys_catalog_->tablet_replica()->consensus()->ConsensusState(&cstate);
+ if (PREDICT_FALSE(!s.ok())) {
+ DCHECK(s.IsIllegalState()) << s.ToString();
+ catalog_status_ = s.CloneAndPrepend("ConsensusState is not available");
+ return;
+ }
+
catalog_status_ = Status::OK();
// Check if the catalog manager is the leader.
- const ConsensusStatePB cstate = catalog_->sys_catalog_->tablet_replica()->
- consensus()->ConsensusState();
initial_term_ = cstate.current_term();
const string& uuid = catalog_->master_->fs_manager()->uuid();
if (PREDICT_FALSE(cstate.leader_uuid() != uuid)) {
@@ -4307,9 +4315,8 @@ CatalogManager::ScopedLeaderSharedLock::ScopedLeaderSharedLock(
bool CatalogManager::ScopedLeaderSharedLock::has_term_changed() const {
DCHECK(leader_status().ok());
- const ConsensusStatePB cstate = catalog_->sys_catalog_->tablet_replica()->
- consensus()->ConsensusState();
- return cstate.current_term() != initial_term_;
+ const auto current_term = catalog_->sys_catalog_->tablet_replica()->consensus()->CurrentTerm();
+ return current_term != initial_term_;
}
template<typename RespClass>
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/master/sys_catalog.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc
index 7836380..355b8f6 100644
--- a/src/kudu/master/sys_catalog.cc
+++ b/src/kudu/master/sys_catalog.cc
@@ -60,6 +60,7 @@
#include "kudu/gutil/bind_helpers.h"
#include "kudu/gutil/gscoped_ptr.h"
#include "kudu/gutil/move.h"
+#include "kudu/gutil/port.h"
#include "kudu/gutil/strings/join.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/master/catalog_manager.h"
@@ -325,7 +326,12 @@ void SysCatalogTable::SysCatalogStateChanged(const string& tablet_id, const stri
<< tablet_id << ". Reason: " << reason;
return;
}
- consensus::ConsensusStatePB cstate = consensus->ConsensusState();
+ consensus::ConsensusStatePB cstate;
+ Status s = consensus->ConsensusState(&cstate);
+ if (PREDICT_FALSE(!s.ok())) {
+ LOG_WITH_PREFIX(WARNING) << s.ToString();
+ return;
+ }
LOG_WITH_PREFIX(INFO) << "SysCatalogTable state changed. Reason: " << reason << ". "
<< "Latest consensus state: " << SecureShortDebugString(cstate);
RaftPeerPB::Role new_role = GetConsensusRole(tablet_replica_->permanent_uuid(), cstate);
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tablet/tablet_replica.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_replica.h b/src/kudu/tablet/tablet_replica.h
index bf3476d..c2f72ba 100644
--- a/src/kudu/tablet/tablet_replica.h
+++ b/src/kudu/tablet/tablet_replica.h
@@ -185,6 +185,11 @@ class TabletReplica : public RefCountedThreadSafe<TabletReplica>,
return state_;
}
+ const TabletDataState data_state() const {
+ std::lock_guard<simple_spinlock> lock(lock_);
+ return meta_->tablet_data_state();
+ }
+
std::string StateName() const;
// Returns the current Raft configuration.
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tserver/tablet_copy_client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_client-test.cc b/src/kudu/tserver/tablet_copy_client-test.cc
index 7ac12cd..7f3b025 100644
--- a/src/kudu/tserver/tablet_copy_client-test.cc
+++ b/src/kudu/tserver/tablet_copy_client-test.cc
@@ -116,7 +116,8 @@ class TabletCopyClientTest : public TabletCopyTest {
messenger_,
nullptr /* no metrics */));
RaftPeerPB* cstate_leader;
- ConsensusStatePB cstate = tablet_replica_->consensus()->ConsensusState();
+ ConsensusStatePB cstate;
+ ASSERT_OK(tablet_replica_->consensus()->ConsensusState(&cstate));
ASSERT_OK(GetRaftConfigLeader(&cstate, &cstate_leader));
leader_ = *cstate_leader;
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tserver/tablet_copy_source_session.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_copy_source_session.cc b/src/kudu/tserver/tablet_copy_source_session.cc
index e2a5227..18bb5fb 100644
--- a/src/kudu/tserver/tablet_copy_source_session.cc
+++ b/src/kudu/tserver/tablet_copy_source_session.cc
@@ -29,7 +29,6 @@
#include "kudu/consensus/log.h"
#include "kudu/consensus/log.pb.h"
#include "kudu/consensus/log_reader.h"
-#include "kudu/consensus/metadata.pb.h"
#include "kudu/consensus/opid.pb.h"
#include "kudu/consensus/opid_util.h"
#include "kudu/consensus/raft_consensus.h"
@@ -189,7 +188,8 @@ Status TabletCopySourceSession::InitOnce() {
"Raft Consensus is not available. Tablet state: $1 ($2)",
tablet_id, tablet::TabletStatePB_Name(tablet_state), tablet_state));
}
- initial_cstate_ = consensus->ConsensusState();
+ RETURN_NOT_OK_PREPEND(consensus->ConsensusState(&initial_cstate_),
+ "Consensus state not available");
// Re-anchor on the highest OpId that was in the log right before we
// snapshotted the log segments. This helps ensure that we don't end up in a
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tserver/tablet_server-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc
index de498dc..c936aea 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -51,6 +51,7 @@
#include "kudu/consensus/log-test-base.h"
#include "kudu/consensus/log.h"
#include "kudu/consensus/metadata.pb.h"
+#include "kudu/consensus/raft_consensus.h"
#include "kudu/fs/block_id.h"
#include "kudu/fs/fs-test-util.h"
#include "kudu/fs/fs.pb.h"
@@ -107,6 +108,7 @@
using google::protobuf::util::MessageDifferencer;
using kudu::clock::Clock;
using kudu::clock::HybridClock;
+using kudu::consensus::ConsensusStatePB;
using kudu::fs::CreateCorruptBlock;
using kudu::pb_util::SecureDebugString;
using kudu::pb_util::SecureShortDebugString;
@@ -359,6 +361,24 @@ TEST_F(TabletServerTest, TestWebPages) {
#endif
}
+// Ensure that when a replica is in a failed / shutdown state, it returns an
+// error for ConsensusState() requests.
+TEST_F(TabletServerTest, TestFailedTabletsRejectConsensusState) {
+ scoped_refptr<TabletReplica> replica;
+ TSTabletManager* tablet_manager = mini_server_->server()->tablet_manager();
+ ASSERT_TRUE(tablet_manager->LookupTablet(kTabletId, &replica));
+ replica->SetError(Status::IOError("This error will leave the replica FAILED state at shutdown"));
+ replica->Shutdown();
+ ASSERT_EQ(tablet::FAILED, replica->state());
+
+ auto consensus = replica->shared_consensus();
+ ASSERT_TRUE(consensus);
+ ConsensusStatePB cstate;
+ Status s = consensus->ConsensusState(&cstate);
+ ASSERT_TRUE(s.IsIllegalState()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "Tablet replica is shutdown");
+}
+
// Test that tablets that get failed and deleted will eventually show up as
// failed tombstones on the web UI.
TEST_F(TabletServerTest, TestFailedTabletsOnWebUI) {
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index 22753d8..dc89823 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -49,7 +49,6 @@
#include "kudu/common/wire_protocol.h"
#include "kudu/common/wire_protocol.pb.h"
#include "kudu/consensus/consensus.pb.h"
-#include "kudu/consensus/metadata.pb.h"
#include "kudu/consensus/opid.pb.h"
#include "kudu/consensus/raft_consensus.h"
#include "kudu/consensus/time_manager.h"
@@ -1212,9 +1211,14 @@ void ConsensusServiceImpl::GetConsensusState(const consensus::GetConsensusStateR
continue;
}
- auto* tablet_info = resp->add_tablets();
- tablet_info->set_tablet_id(replica->tablet_id());
- *tablet_info->mutable_cstate() = consensus->ConsensusState();
+ consensus::GetConsensusStateResponsePB_TabletConsensusInfoPB tablet_info;
+ Status s = consensus->ConsensusState(tablet_info.mutable_cstate());
+ if (!s.ok()) {
+ DCHECK(s.IsIllegalState()) << s.ToString();
+ continue;
+ }
+ tablet_info.set_tablet_id(replica->tablet_id());
+ *resp->add_tablets() = std::move(tablet_info);
}
context->RespondSuccess();
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tserver/ts_tablet_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/ts_tablet_manager.cc b/src/kudu/tserver/ts_tablet_manager.cc
index bef2d9d..4d2558b 100644
--- a/src/kudu/tserver/ts_tablet_manager.cc
+++ b/src/kudu/tserver/ts_tablet_manager.cc
@@ -172,6 +172,7 @@ namespace kudu {
using consensus::ConsensusMetadata;
using consensus::ConsensusMetadataCreateMode;
using consensus::ConsensusMetadataManager;
+using consensus::ConsensusStatePB;
using consensus::OpId;
using consensus::OpIdToString;
using consensus::RECEIVED_OPID;
@@ -1159,7 +1160,11 @@ void TSTabletManager::CreateReportedTabletPB(const scoped_refptr<TabletReplica>&
auto include_health = FLAGS_raft_prepare_replacement_before_eviction ?
RaftConsensus::INCLUDE_HEALTH_REPORT :
RaftConsensus::EXCLUDE_HEALTH_REPORT;
- *reported_tablet->mutable_consensus_state() = consensus->ConsensusState(include_health);
+ ConsensusStatePB cstate;
+ Status s = consensus->ConsensusState(&cstate, include_health);
+ if (PREDICT_TRUE(s.ok())) {
+ *reported_tablet->mutable_consensus_state() = std::move(cstate);
+ }
}
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/e605c162/src/kudu/tserver/tserver_path_handlers.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tserver_path_handlers.cc b/src/kudu/tserver/tserver_path_handlers.cc
index 3dd602f..514ce14 100644
--- a/src/kudu/tserver/tserver_path_handlers.cc
+++ b/src/kudu/tserver/tserver_path_handlers.cc
@@ -211,6 +211,10 @@ string TabletLink(const string& id) {
EscapeForHtmlToString(id));
}
+bool IsTombstoned(const scoped_refptr<TabletReplica>& replica) {
+ return replica->data_state() == tablet::TABLET_DATA_TOMBSTONED;
+}
+
} // anonymous namespace
void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest& /*req*/,
@@ -286,7 +290,16 @@ void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest& /*
.PartitionDebugString(replica->tablet_metadata()->partition(),
replica->tablet_metadata()->schema());
+ // We don't show the config if it's a tombstone because it's misleading.
+ string consensus_state_html;
shared_ptr<consensus::RaftConsensus> consensus = replica->shared_consensus();
+ if (!IsTombstoned(replica) && consensus) {
+ ConsensusStatePB cstate;
+ if (consensus->ConsensusState(&cstate).ok()) {
+ consensus_state_html = ConsensusStatePBToHtml(cstate);
+ }
+ }
+
*output << Substitute(
// Table name, tablet id, partition
"<tr><td>$0</td><td>$1</td><td>$2</td>"
@@ -296,7 +309,7 @@ void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest& /*
tablet_id_or_link, // $1
EscapeForHtmlToString(partition), // $2
EscapeForHtmlToString(replica->HumanReadableState()), mem_bytes, n_bytes, // $3, $4, $5
- consensus ? ConsensusStatePBToHtml(consensus->ConsensusState()) : "", // $6
+ consensus_state_html, // $6
EscapeForHtmlToString(status.last_status())); // $7
}
*output << "<tbody></table>\n</div>\n";
[3/3] kudu git commit: [tablet_service] remove useless call
Posted by al...@apache.org.
[tablet_service] remove useless call
Removed useless call of TabletReplica::permanent_uuid()
in ConsensusServiceImpl::UpdateConsensus().
Change-Id: I3869b423a7a8f32b64201ec83d8ad4d61e2ee9f6
Reviewed-on: http://gerrit.cloudera.org:8080/9324
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
(cherry picked from commit 0396cca5bd7e2ba6b038813ed70fd82c707d9861)
Reviewed-on: http://gerrit.cloudera.org:8080/9550
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/3aa93319
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/3aa93319
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/3aa93319
Branch: refs/heads/branch-1.6.x
Commit: 3aa9331918c51a0447b229a16d01d4455dd5e799
Parents: c1fd2bc
Author: Alexey Serbin <as...@cloudera.com>
Authored: Wed Feb 14 14:09:39 2018 -0800
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Fri Mar 9 20:06:04 2018 +0000
----------------------------------------------------------------------
src/kudu/tserver/tablet_service.cc | 2 --
1 file changed, 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/3aa93319/src/kudu/tserver/tablet_service.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index dc89823..9d47465 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -934,8 +934,6 @@ void ConsensusServiceImpl::UpdateConsensus(const ConsensusRequestPB* req,
return;
}
- replica->permanent_uuid();
-
// Submit the update directly to the TabletReplica's RaftConsensus instance.
shared_ptr<RaftConsensus> consensus;
if (!GetConsensusOrRespond(replica, resp, context, &consensus)) return;
[2/3] kudu git commit: [tablet] fix nullptr dereference while
capturing iterators
Posted by al...@apache.org.
[tablet] fix nullptr dereference while capturing iterators
Added a check into Tablet::CaptureConsistentIterators() to make sure
the tablet is not stopped/shutdown.
Before this patch in one test scenario I saw stack traces
like below (built in DEBUG configuration):
kudu-tserver: src/kudu/gutil/ref_counted.h:284: T *scoped_refptr<kudu::tablet::TabletComponents>::operator->() const [T = kudu::tablet::TabletComponents]: Assertion `ptr_ != __null' failed.
*** Aborted at 1517534012 (unix time) try "date -d @1517534012" if you are using GNU date ***
PC: @ 0x7ff9ad39cc37 gsignal
*** SIGABRT (@0x3e80000745f) received by PID 29791 (TID 0x7ff99a0bc700) from PID 29791; stack trace: ***
@ 0x7ff9b5129330 (unknown) at ??:0
@ 0x7ff9ad39cc37 gsignal at ??:0
@ 0x7ff9ad3a0028 abort at ??:0
@ 0x7ff9ad395bf6 (unknown) at ??:0
@ 0x7ff9ad395ca2 __assert_fail at ??:0
@ 0x7ff9b7f2ce52 scoped_refptr<>::operator->() at ??:0
@ 0x7ff9b7f1bf6d kudu::tablet::Tablet::CaptureConsistentIterators() at ??:0
@ 0x7ff9b7f225f6 kudu::tablet::Tablet::Iterator::Init() at ??:0
@ 0x7ff9b94372e3 kudu::tserver::TabletServiceImpl::HandleNewScanRequest() at ??:0
@ 0x7ff9b943a906 kudu::tserver::TabletServiceImpl::Checksum() at ??:0
@ 0x7ff9b3d3c83d kudu::tserver::TabletServerServiceIf::TabletServerServiceIf()::$_11::operator()() at ??:0
@ 0x7ff9b3d3c682 std::_Function_handler<>::_M_invoke() at ??:0
@ 0x7ff9b2ea026b std::function<>::operator()() at ??:0
@ 0x7ff9b2e9fb2d kudu::rpc::GeneratedServiceIf::Handle() at ??:0
@ 0x7ff9b2ea1ee6 kudu::rpc::ServicePool::RunThread() at ??:0
@ 0x7ff9b2ea4499 boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7ff9b2ea4400 boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7ff9b2ea43aa boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7ff9b2ea418d boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7ff9b2e45f68 boost::function0<>::operator()() at ??:0
@ 0x7ff9b115162d kudu::Thread::SuperviseThread() at ??:0
@ 0x7ff9b5121184 start_thread at ??:0
@ 0x7ff9ad463ffd clone at ??:0
@ 0x0 (unknown)
I used the following WIP stress test for the reproduction scenario:
https://gerrit.cloudera.org/#/c/9255/
For DEBUG builds, without fix the issues appeared ~0.5% of cases. After
the fix, the issue could not be reproduced:
Without fix:
http://dist-test.cloudera.org//job?job_id=aserbin.1518492521.137030
With fix:
http://dist-test.cloudera.org//job?job_id=aserbin.1518492937.141401
Change-Id: Ia7600f006c8df7f445cc2551e99390177378bcff
Reviewed-on: http://gerrit.cloudera.org:8080/9189
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
(cherry picked from commit 5d10a56f9d06dc695f2a4469edbabce978912eb4)
Reviewed-on: http://gerrit.cloudera.org:8080/9549
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c1fd2bc4
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c1fd2bc4
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c1fd2bc4
Branch: refs/heads/branch-1.6.x
Commit: c1fd2bc428f0778e865739d48cdbc5e828fda6ca
Parents: e605c16
Author: Alexey Serbin <as...@cloudera.com>
Authored: Thu Feb 1 19:21:07 2018 -0800
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Fri Mar 9 20:06:00 2018 +0000
----------------------------------------------------------------------
src/kudu/tablet/tablet.cc | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/c1fd2bc4/src/kudu/tablet/tablet.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc
index 29596b2..293a0a4 100644
--- a/src/kudu/tablet/tablet.cc
+++ b/src/kudu/tablet/tablet.cc
@@ -1722,21 +1722,23 @@ Status Tablet::DebugDump(vector<string> *lines) {
}
Status Tablet::CaptureConsistentIterators(
- const Schema *projection,
- const MvccSnapshot &snap,
- const ScanSpec *spec,
- OrderMode order,
- vector<shared_ptr<RowwiseIterator> > *iters) const {
+ const Schema* projection,
+ const MvccSnapshot& snap,
+ const ScanSpec* spec,
+ OrderMode order,
+ vector<shared_ptr<RowwiseIterator>>* iters) const {
+
shared_lock<rw_spinlock> l(component_lock_);
+ RETURN_IF_STOPPED_OR_CHECK_STATE(kOpen);
// Construct all the iterators locally first, so that if we fail
// in the middle, we don't modify the output arguments.
- vector<shared_ptr<RowwiseIterator> > ret;
+ vector<shared_ptr<RowwiseIterator>> ret;
// Grab the memrowset iterator.
gscoped_ptr<RowwiseIterator> ms_iter;
RETURN_NOT_OK(components_->memrowset->NewRowIterator(projection, snap, order, &ms_iter));
- ret.push_back(shared_ptr<RowwiseIterator>(ms_iter.release()));
+ ret.emplace_back(ms_iter.release());
// Cull row-sets in the case of key-range queries.
if (spec != nullptr && spec->lower_bound_key() && spec->exclusive_upper_bound_key()) {
@@ -1754,7 +1756,7 @@ Status Tablet::CaptureConsistentIterators(
RETURN_NOT_OK_PREPEND(rs->NewRowIterator(projection, snap, order, &row_it),
Substitute("Could not create iterator for rowset $0",
rs->ToString()));
- ret.push_back(shared_ptr<RowwiseIterator>(row_it.release()));
+ ret.emplace_back(row_it.release());
}
ret.swap(*iters);
return Status::OK();
@@ -1767,7 +1769,7 @@ Status Tablet::CaptureConsistentIterators(
RETURN_NOT_OK_PREPEND(rs->NewRowIterator(projection, snap, order, &row_it),
Substitute("Could not create iterator for rowset $0",
rs->ToString()));
- ret.push_back(shared_ptr<RowwiseIterator>(row_it.release()));
+ ret.emplace_back(row_it.release());
}
// Swap results into the parameters.