Andrew Wong 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? 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 readers don't need to jump between the def and usage. The details also make a lot more sense closer to the usage -- it's otherwise a bit unclear how callers should use this function. 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 !(allow_status_msg_for_failed_peer_ && *allow_status_msg_for_peer_ && !peer.wal_catchup_possible) Alternatively invert the entire thing and have name the function ShouldSendStatusOnlyMessages 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 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 injected failure? Elsewhere we just use "INJECTED FAILURE". -- 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: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 04 Jan 2021 07:05:23 +0000 Gerrit-HasComments: Yes
