Zoltan Martonka 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: (19 comments) 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 consiste Done 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 Done 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 Done 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/coul added a comment line 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? No, just the snooze todo. 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? No, it is left here from debugging on older version, when the if was missing the !res_has_ops condition, and the &ops_pending parameter for RequestForPeer was badly implemented. 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 Done 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 Done 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? Done 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? Done 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? Done http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@136 PS20, Line 136: tserver > nit: TServer Done http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_batcher.h@169 PS20, Line 169: tserver > nit: TServer Done 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 w Done, thank you for the suggestion. 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? I need some upper limit, delay should not be comparable to the heartbeat interval. Added a comment. 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? Done 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 Done 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)? Done http://gerrit.cloudera.org:8080/#/c/22867/20/src/kudu/consensus/multi_raft_consensus_data.h@42 PS20, Line 42: rpc > nit: RPC Done -- 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 13:59:17 +0000 Gerrit-HasComments: Yes
