Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/22867 )
Change subject: KUDU-1973: Send no-op heartbeat operations batched - PART 1 ...................................................................... Patch Set 20: Code-Review+1 (20 comments) small nits only http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_peers-test.cc File src/kudu/consensus/consensus_peers-test.cc: http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_peers-test.cc@292 PS20, Line 292: nullptr here and below, maybe put the same comment here as above, just for consistency http://gerrit.cloudera.org:8080/#/c/22867/17/src/kudu/consensus/consensus_peers.h File src/kudu/consensus/consensus_peers.h: http://gerrit.cloudera.org:8080/#/c/22867/17/src/kudu/consensus/consensus_peers.h@146 PS17, Line 146: void ProcessSingleResponse(); nit: missing empty lines above and below http://gerrit.cloudera.org:8080/#/c/22867/12/src/kudu/consensus/consensus_peers.h File src/kudu/consensus/consensus_peers.h: http://gerrit.cloudera.org:8080/#/c/22867/12/src/kudu/consensus/consensus_peers.h@84 PS12, Line 84: s nit: missing dot http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_peers.cc@264 PS20, Line 264: // We don't want to send out a heavy request on the batching thread. what constitutes as a heavy request here? is there a limit that should/could be checked? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_peers.cc@327 PS20, Line 327: if (multi_raft_batcher_) { : // TODO(martonka) needs lock free snooze. : // DCHECK(mrc_data == nullptr) << "Messages with ops should not be sent on MRC thread"; : // Should be always true. But could cause a deadlock if we are on the MRC thread. : // if (PREDICT_TRUE(!mrc_data)) { : // DCHECK(multi_raft_batcher_registration_); : // multi_raft_batcher_->Snooze(multi_raft_batcher_registration_.value()); : // } : } else { is this part needed? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_peers.cc@363 PS20, Line 363: DCHECK(!req_has_ops); is this DCHECK necessary? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/consensus_queue.cc@682 PS20, Line 682: nit: unnecessary empty line http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h File src/kudu/consensus/multi_raft_batcher.h: http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@57 PS20, Line 57: tbatching nit: typo http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@62 PS20, Line 62: (average over a long time will be raft_heartbeat_interval_ms). this whole sentence is in parentheses are they really needed? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@93 PS20, Line 93: // Collect the heartbeaters that are due to be called in the next flush interval : // and batch them together, and submit a task to the raft pool to actually generate : // and send out the no-op heartbeats. nit: maybe break this up to a list instead of a single sentence? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@113 PS20, Line 113: Just do a round-robin style call on the subscribed heartbeaters : // whenever we call a peer we push it to the end of the queue : // with the next required time. nit: maybe reformat this for readability? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@136 PS20, Line 136: tserver nit: TServer http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@169 PS20, Line 169: tserver nit: TServer http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.cc File src/kudu/consensus/multi_raft_batcher.cc: http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.cc@58 PS20, Line 58: For minimal delay and still maximum effectiveness set : // multi_raft_heartbeat_window_ms = heartbeat_interval_ms * ( : // batch_size / {estimated follower peers from the same host}) + {a little tolerance}; : // however, a 0.1 * heartbeat_interval_ms is good, and there is no reason to : // set it any lower. : // This value is also forced to be less than or equal to half of the heartbeat interval, : // because it makes no sense to introduce a possible delay comparable to the heartbeat interval maybe this should be in the flag description? that way the generated docs would include these details http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.cc@226 PS20, Line 226: FLAGS_consensus_rpc_timeout_ms / 2 why are you comparing with the half of the rpc timeout? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.cc@276 PS20, Line 276: std::shared_ptr<ThreadPoolToken>(move(raft_pool_->NewToken(ThreadPool::ExecutionMode::CONCURRENT))); nit: line too long? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_consensus_data.h File src/kudu/consensus/multi_raft_consensus_data.h: http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_consensus_data.h@30 PS20, Line 30: nit: unnecessary empty line http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_consensus_data.h@31 PS20, Line 31: // Data for a single multi-raft consensus batch. what data is this line referring to? maybe a duplicate (line #39)? http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_consensus_data.h@42 PS20, Line 42: the nit: The http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_consensus_data.h@42 PS20, Line 42: rpc nit: RPC -- To view, visit http://gerrit.cloudera.org:8080/22867 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie92ba4de5eae00d56cd513cb644dce8fb6e14538 Gerrit-Change-Number: 22867 Gerrit-PatchSet: 20 Gerrit-Owner: Zoltan Martonka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Mon, 29 Sep 2025 12:11:41 +0000 Gerrit-HasComments: Yes
