You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/10/30 00:28:13 UTC

[kudu-CR] raft consensus quorum-test: avoid thread collision warner crash

Hello Alexey Serbin, Volodymyr Verovkin,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/14580

to review the following change.


Change subject: raft_consensus_quorum-test: avoid thread collision warner crash
......................................................................

raft_consensus_quorum-test: avoid thread collision warner crash

One of my precommits failed with ThreadCollisionWarner triggering a crash in
raft_consensus_quorum-test. Digging into it, the cause appears to be the
"synthetic" nature that the test checks certain cmeta properties.

Back in commit 17f97531e the real cmeta mutex was replaced with a
DFAKE_MUTEX. This fake lock works by using the ThreadCollisionWarner to
enforce that access to the "protected" object is externally synchronized;
that is, another lock is always held while calling the object's methods. In
cmeta's case, that external lock is the main RaftConsensus lock.

The problem is that raft_consensus_quorum-test accesses cmeta directly,
without going through RaftConsensus. This opens the door to crashes like
these since there's no external synchronization in these accesses.

I opted to fix this by changing the test's ReadConsensusMetadataFromDisk
method to actually go to disk for its cmeta. I could have poked a few test
holes into RaftConsensus, but it seemed odd for a method that purported to
go "to disk" to actually get cmeta from the ConsensusMetadataManager, which
caches it in a map and always returns the same instance.

I also included some test cleanup.

  I1029 18:08:21.149920   222 raft_consensus.cc:2316] T TestTablet P 1894ac4aabfc4698b93d6c621f3c9ceb [term 1 FOLLOWER]: Leader election vote request: Denying vote to candidate c0fad19d10b74845b028ecce5cde3fbe for term 2 because replica is either leader or believes a valid leader to be alive.
  I1029 18:08:21.150341   222 raft_consensus.cc:2901] T TestTablet P 1894ac4aabfc4698b93d6c621f3c9ceb [term 1 FOLLOWER]: Advancing to term 2
  W1029 18:08:21.701403   222 consensus_meta.cc:220] T TestTablet P 1894ac4aabfc4698b93d6c621f3c9ceb: Time spent flushing consensus metadata: real 0.551s	user 0.001s	sys 0.000s
  I1029 18:08:21.701627   222 raft_consensus.cc:2355] T TestTablet P 1894ac4aabfc4698b93d6c621f3c9ceb [term 2 FOLLOWER]: Leader election vote request: Granting yes vote for candidate c0fad19d10b74845b028ecce5cde3fbe in term 2.
  F1029 18:08:21.701889   586 thread_collision_warner.cc:23] Thread Collision! Previous thread id: 222, current thread id: 586
  *** Check failure stack trace: ***
  *** Aborted at 1572372501 (unix time) try "date -d @1572372501" if you are using GNU date ***
  PC: @     0x7ff8b8713c37 gsignal
  *** SIGABRT (@0x3e8000000de) received by PID 222 (TID 0x7ff8a2b0e700) from PID 222; stack trace: ***
    @     0x7ff8b92d6330 (unknown) at ??:0
    @     0x7ff8b8713c37 gsignal at ??:0
    @     0x7ff8b8717028 abort at ??:0
    @     0x7ff8bb112e09 google::logging_fail() at ??:0
    @     0x7ff8bb11462d google::LogMessage::Fail() at ??:0
    @     0x7ff8bb11664c google::LogMessage::SendToLog() at ??:0
    @     0x7ff8bb114189 google::LogMessage::Flush() at ??:0
    @     0x7ff8bb116fdf google::LogMessageFatal::~LogMessageFatal() at ??:0
    @     0x7ff8bb4ef2d3 base::DCheckAsserter::warn() at ??:0
    @     0x7ff8bb4ef3e8 base::ThreadCollisionWarner::EnterSelf() at ??:0
    @     0x7ff8c6ecb86b kudu::consensus::ConsensusMetadata::current_term() at ??:0
    @     0x7ff8c6fa1632 kudu::consensus::RaftConsensus::CurrentTermUnlocked() at ??:0
    @     0x7ff8c6fc847e kudu::consensus::RaftConsensus::HandleLeaderRequestTermUnlocked() at ??:0
    @     0x7ff8c6fca887 kudu::consensus::RaftConsensus::CheckLeaderRequestUnlocked() at ??:0
    @     0x7ff8c6fbf7c7 kudu::consensus::RaftConsensus::UpdateReplica() at ??:0
    @     0x7ff8c6fbe5e2 kudu::consensus::RaftConsensus::Update() at ??:0
    @           0x6112b2 kudu::consensus::LocalTestPeerProxy::SendUpdateRequest() at /home/jenkins-slave/workspace/kudu-master/2/src/kudu/consensus/consensus-test-util.h:?

Change-Id: Icacdef44fd545f46eb9f47c577641e3aa66d6e94
---
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
5 files changed, 32 insertions(+), 36 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/14580/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14580
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icacdef44fd545f46eb9f47c577641e3aa66d6e94
Gerrit-Change-Number: 14580
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>