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 11: (24 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-1973 > Is this the same as KUDU-1973? yes http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@20 PS9, Line 20: the CPU load. We onl > nit: allows for the following removed http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@35 PS9, Line 35: beats are batch > Were those RELEASE or DEBUG binaries? I'd guess those were RELEASE ones, r release http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@37 PS9, Line 37: e 1 timer call and [1, X+1] callbacks o > Did you sample that only for a particular tablet server or that's the stats 1 ts only. I checked that the tablets/leaders were well distributed. http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@39 PS9, Line 39: e there > nit: which exact flag? would be great to mention enable_multi_raft_heartbeat_batcher http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@40 PS9, Line 40: for flush/discard logic. > BTW, did you happen to look at the locking primitives usage? Anything like Added stat_runtime and switch to the commit message. I have all the others (sched:sched_*) saved as well, so I can check any of them without redoing the measurements. http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@44 PS9, Line 44: he response on a sin > nit: seems to be a duplication? Done http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@47 PS9, Line 47: M > nit for here and below: replace tabs with spaces for consistent formatting Done 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 Done http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus.proto@422 PS9, Line 422: required > There was an intent to switch to protobuf3 eventually, and having 'required It would be fine to have them optional, but I would like to keep them exactly the same as their non-batched counterparts. 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 ba replied in top level comment. 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: rtual void UpdateAsync(const Consensu > nit: PENDING_BUFFERED_POSSIBLY_IN_BUFFER without the "ED"? removed http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.h@264 PS9, Line 264: // Instructs a peer to begin a tablet copy session. > nit: shouldn't this be moved with the rest of the methods? removed 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 Done http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/consensus_peers.cc@451 PS9, Line 451: // Note: This method runs on the reactor thread. > We are holding the peer_lock_, so relaxed is enough cleaned up this mess with changing the logic. 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@70 PS9, Line 70: = d > nit: do removed http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.h@155 PS9, Line 155: > nit: deletion? Done 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: > If we know the expected size of the vector, shouldn't we reserve capacity t It definitely doesn’t hurt to reserve. http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@166 PS9, Line 166: std::shared_ptr<MultiRaftConsensusData> data) { > Should these be removed? removed http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@186 PS9, Line 186: > nit: extra space removed http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@194 PS9, Line 194: cb.heartbeater.heartbeat_callback(data.get()); > Is it a valid scenario when the request doesn't have a caller UUID and a de Yes, they should be always set. Fixed. http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/consensus/multi_raft_batcher.cc@278 PS9, Line 278: > nit: space after colon Done 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 Done http://gerrit.cloudera.org:8080/#/c/22867/9/src/kudu/tserver/tablet_service.cc@1815 PS9, Line 1815: b > nit: missing space before colon 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: 11 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: Tue, 29 Jul 2025 16:51:17 +0000 Gerrit-HasComments: Yes
