Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9627 )
Change subject: WIP: KUDU-2342. consensus: use tighter bound for non-voter promotion ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/9627/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/9627/1/src/kudu/consensus/consensus_queue.cc@913 PS1, Line 913: TrackedPeer* peer, > parameter ordering nit: maybe, place this 'out' parameter after the 'in' on It's a grey area based on my reading of the GSG. Since this is acting on a struct 'peer' that is not really an output parameter but more of a mutable input parameter or maybe something kind of like an in-out parameter (though not purely so), I think it's clearer with the primary thing we're operating on as the first argument. If you feel strongly that this is confusing let me know and I can put it after the const ref input-only parameters. Conceptually I think of this as kind of a method of the TrackedPeer struct, even though that's not really the case. http://gerrit.cloudera.org:8080/#/c/9627/1/src/kudu/consensus/consensus_queue.cc@958 PS1, Line 958: const ConsensusStatusPB& status) { > nit: maybe, add a DCHECK() to make sure the necessary lock are held while e Done http://gerrit.cloudera.org:8080/#/c/9627/1/src/kudu/consensus/consensus_queue.cc@974 PS1, Line 974: prev_peer_state.last_received.index() > should we ignore this if it's -1? I don't think this is possible (see below) http://gerrit.cloudera.org:8080/#/c/9627/1/src/kudu/consensus/consensus_queue.cc@976 PS1, Line 976: status.last_received_current_leader() > Sometimes I see OpId index equal to -1. Should we ignore such cases as wel everywhere that I looked, last_received_current_leader() either gives MinimumOpId() or a real OpId. -1 is kInvalidOpIdIndex and will never be in an actual OpId. http://gerrit.cloudera.org:8080/#/c/9627/1/src/kudu/consensus/consensus_queue.cc@977 PS1, Line 977: last_batch_size > Is this safe enough if the peer was receiving heartbeats from some strayed if this is the first time we talked to this peer then prev_peer_state.last_received should be (0, 0), and you're right we're missing handling for that. Added. -- To view, visit http://gerrit.cloudera.org:8080/9627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff517f01d6dc25eb15d01593dd57b7dc0dd25956 Gerrit-Change-Number: 9627 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 14 Mar 2018 23:22:55 +0000 Gerrit-HasComments: Yes
