Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/22867 )
Change subject: KUDU-3665 Send no-op heartbeat operations batched PART1 ...................................................................... Patch Set 9: (19 comments) http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@7 PS9, Line 7: KUDU-3665 Is this the same as KUDU-1973? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus.proto@420 PS9, Line 420: // messages nit here and below: missing period at the end of the sentence http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers-test.cc File src/kudu/consensus/consensus_peers-test.cc: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers-test.cc@151 PS9, Line 151: nullptr, // We do not use heartbeat batching should we use parameterized tests instead to check both with and without batching? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.h File src/kudu/consensus/consensus_peers.h: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.h@247 PS9, Line 247: PENDING_BUFFERED_POSSIBLY_IN_BUFFERED nit: PENDING_BUFFERED_POSSIBLY_IN_BUFFER without the "ED"? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.h@264 PS9, Line 264: bool CheckPendingAndDiscardBuffered(bool periodic_req); nit: shouldn't this be moved with the rest of the methods? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.cc@221 PS9, Line 221: nit: remove extra indent http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.cc@451 PS9, Line 451: CHECK(request_pending_.load(std::memory_order_relaxed) != RequestStatus::NO_ACTIVE); Why is it memory_order_relaxed in this CHECK while it's not in others? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.h File src/kudu/consensus/multi_raft_batcher.h: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.h@58 PS9, Line 58: : nit here and below: in inheritance, there should be a space on both sides of the colon http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.h@70 PS9, Line 70: does nit: do http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.h@133 PS9, Line 133: hostport nit: HostPort http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.h@155 PS9, Line 155: decation nit: deletion? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc File src/kudu/consensus/multi_raft_batcher.cc: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@118 PS9, Line 118: // Buffer will be 10-20 messages, so a vector is more efficient than a map. If we know the expected size of the vector, shouldn't we reserve capacity to e.g. 32 to avoid costly reallocations? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@166 PS9, Line 166: //PrepareAndSendBatchRequest(); Should these be removed? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@186 PS9, Line 186: nit: extra space Also, should we add some extra info to the log message? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@194 PS9, Line 194: DCHECK(request->caller_uuid() == current_batch_->batch_req.caller_uuid()); Is it a valid scenario when the request doesn't have a caller UUID and a destination UUID? Shouldn't these DCHECKs be outside of these tests? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@278 PS9, Line 278: : nit: space after colon http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/raft_consensus_quorum-test.cc File src/kudu/consensus/raft_consensus_quorum-test.cc: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/raft_consensus_quorum-test.cc@241 PS9, Line 241: nullptr, should we use parameterized tests instead to check both with and without batching? http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/tserver/tablet_service.cc@269 PS9, Line 269: res nit: res seems a bit ambiguous as it sounds like a shorthand for response http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/tserver/tablet_service.cc@1815 PS9, Line 1815: : nit: missing space before colon -- 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: 9 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: Fri, 04 Jul 2025 21:52:46 +0000 Gerrit-HasComments: Yes
