David Ribeiro Alves has posted comments on this change. Change subject: [consensus] KUDU-1718: Fix few bugs around replica eviction failures ......................................................................
Patch Set 2: (4 comments) can you please address the tidy bot nits? I know most are not your fault but would be nice to fix them anyway http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: PS2, Line 235: // Analyze the response from peer. : PeerResponseStatus ps = AnalyzePeerResponse(); : if (PREDICT_FALSE(ps.response != PeerResponseStatus::OK)) { : VLOG_WITH_PREFIX_UNLOCKED(1) << "Error response from peer: " << ps.status.ToString() : << ": " << response_.ShortDebugString(); : if (ps.response == PeerResponseStatus::ERROR_BUT_ALIVE) { : queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid()); : } : ProcessResponseError(ps.status); can you change this for a switch case where the error cases get a fall through? something like: switch (ps) { case ERROR_BUT_ALIVE: queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid()); // Fallthrough intended. case ERROR: ProcessResponseError(ps.status); return; case OK: // The queue's handling of the peer response may generate IO (reads against // the WAL) and SendNextRequest() may do the same thing. So we run the rest // of the response handling logic on our thread pool and not on the reactor // thread. Status s = thread_pool_->SubmitClosure(Bind(&Peer::DoProcessResponse, Unretained(this))); if (PREDICT_FALSE(!s.ok())) { LOG_WITH_PREFIX_UNLOCKED(WARNING) << "Unable to process peer response: " << s.ToString() << ": " << response_.ShortDebugString(); sem_.Release(); } http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/consensus/consensus_peers.h File src/kudu/consensus/consensus_peers.h: PS2, Line 58: PeerResponseStatus thanks for adding this. http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: PS2, Line 2695: // Tests KUDU-1613 and KUDU-1407 fixes when followers aren't evicted. please be a little more discriptive http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS2, Line 2589: VLOG(4) << "Error deleting the tablet " << tablet_id() << ": " : << resp_.DebugString(); should we use a higher log level here or is there enough information in the statements below? -- To view, visit http://gerrit.cloudera.org:8080/5111 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0d3f84fd297a8e4760208a213c1ee393e92499a3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
