Todd Lipcon 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: (2 comments) http://gerrit.cloudera.org:8080/#/c/9627/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9627/1//COMMIT_MSG@20 PS1, Line 20: Note: While in consensus_queue.cc I also factored a couple of code was it really necessary to combine the refactor and this bug fix/improvement? I'm finding it hard to see what you actually changed, and I'm not clear that it's a 100% "extract method" (it appears like some of the blocks changed order and I don't know 100% whether that has any side effects from quickly looking at the patch). 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@959 PS1, Line 959: if (queue_state_.mode == PeerMessageQueue::LEADER && : peer->last_exchange_status == PeerStatus::OK) { > readability nit: maybe, return right away if the condition yields false? agreed. Is this also an improvement to the policy? ie it's not just that we are checking the bound, but we're also now checking that it was a successful exchange -- 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: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 14 Mar 2018 20:48:34 +0000 Gerrit-HasComments: Yes
