Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16899 )
Change subject: [consensus] Allow sending status-only request messages to FAILED peer ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue-test.cc File src/kudu/consensus/consensus_queue-test.cc: http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue-test.cc@413 PS1, Line 413: request.mutable_ops()->ExtractSubrange(0, request.ops_size(), nullptr); > I might be missing something -- what's this for? In this case it's not needed with 0 ops. So I'll remove it. http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue.h@551 PS1, Line 551: ShouldSendLogMessagesToPeer > nit: this appears to only be used once, so maybe just inline it? That way r Done http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue.h@561 PS1, Line 561: (allow_status_msg_for_failed_peer_ == nullptr || !*allow_status_msg_for_failed_peer_ || : peer.wal_catchup_possible); > nit: up to you, but I found it much easier to reason about this written as Hmm. I don't find one more readable than the other. I'll keep it as is. http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue.cc@711 PS1, Line 711: fault_ > nit: add PREDICT_FALSE Done http://gerrit.cloudera.org:8080/#/c/16899/1/src/kudu/consensus/consensus_queue.cc@713 PS1, Line 713: Lo > nit: mind adding something that makes this obvious the message is an inject Done -- To view, visit http://gerrit.cloudera.org:8080/16899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I229cc739c1b5ec7b11ce05d5e6b1b8e9d654d6f7 Gerrit-Change-Number: 16899 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 06 Jan 2021 00:30:35 +0000 Gerrit-HasComments: Yes
