David Ribeiro Alves has posted comments on this change. Change subject: [consensus] KUDU-1718: Fix few bugs around replica eviction failures ......................................................................
Patch Set 1: (1 comment) would it be possible to split this patch into several ones that take care of each ticker individually or would that be too hard? http://gerrit.cloudera.org:8080/#/c/5111/1/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: PS1, Line 235: if (!controller_.status().ok()) { : if (controller_.status().IsRemoteError()) { : // Most controller errors are caused by network issues or corner cases : // like shutdown and failure to serialize a protobuf. Therefore, we : // generally consider these errors to indicate an unreachable peer. : // However, a RemoteError wraps some other error propagated from the : // remote peer, so we know the remote is alive. Therefore, we will let : // the queue know that the remote is responsive. : queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid()); : } : ProcessResponseError(controller_.status()); : return; : } : // Again, let the queue know that the remote is still responsive, since we : // will not be sending this error response through to the queue. : // For certain error codes, we want the queue to treat the remote as : // unresponsive and take necessary actions, hence bypassing the notification : // for those error codes. : if (response_.has_error()) { : if (response_.error().code() != TabletServerErrorPB::TABLET_NOT_FOUND && : response_.error().code() != TabletServerErrorPB::WRONG_SERVER_UUID && : response_.error().code() != TabletServerErrorPB::TABLET_NOT_RUNNING) { : queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid()); : ProcessResponseError(StatusFromPB(response_.error().status())); : return; : } : } else if (response_.status().has_error()) { : if (response_.status().error().code() == consensus::ConsensusErrorPB::CANNOT_PREPARE) { : queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid()); : ProcessResponseError(StatusFromPB(response_.error().status())); : return; : } : } It would be good if we could have something like we have in the c++ client where we analize the respose and then get a directive back. Something like: PeerResponseStatus AnalyzeResponse() which would have the nightmarish string of ifs inside and return: OK ERROR ERROR_BUT_RESPONSIVE would that make sense? -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
