Marton Greber 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 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/consensus_peers.cc@330 PS8, Line 330: Swap(new ConsensusStatusPB()) Isn't this causing a memory leak? http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h File src/kudu/consensus/multi_raft_batcher.h: http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h@77 PS8, Line 77: uint64 nit: uint64_t ? http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h@80 PS8, Line 80: uint64 nit: uint64_t ? http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h@125 PS8, Line 125: next_idx nit: next_idx_ http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h@126 PS8, Line 126: buffer_start_idx nit: buffer_start_idx_ http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc File src/kudu/consensus/multi_raft_batcher.cc: http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc@18 PS8, Line 18: #include <kudu/consensus/multi_raft_batcher.h> nit: #include "kudu/consensus/multi_raft_batcher.h" ? http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc@54 PS8, Line 54: // TODO(martonka): remove counters after performance measurements. What do you think about keeping these metrics for observability? Operators can graph no_op_heartbeat_count / heartbeat_batch_count to see the "average batch size". Obviously if we decide to keep the counters, it would be best to move them into member variables. http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc@71 PS8, Line 71: multi_raft_heartbeat_interval_ms >From the warning message below: "multi_raft_heartbeat_interval_ms should be at >most half of heartbeat_interval_ms". Does it make sense to add a validator for this flag? http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc@94 PS8, Line 94: kudu::Counter nit: Counter http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc@119 PS8, Line 119: buffered_msg_ids_ nit: buffered_msg_ids -- 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: 8 Gerrit-Owner: Zoltan Martonka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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, 24 Jun 2025 19:37:00 +0000 Gerrit-HasComments: Yes
