Zoltan Martonka 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: (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: tcher_->AddRequestToBatch( > Isn't this causing a memory leak? It does, and this line is superfluous, response_ is properly cleared at the beginning of the response handling. 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 ? Done http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h@80 PS8, Line 80: uint64 > nit: uint64_t ? Done http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.h@125 PS8, Line 125: next_idx > nit: next_idx_ Done 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_ Done 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" ? Done 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 I think if we keep them, we should change to a simple RPC messages sent counter. You can still use that single counter to see the difference in the number of calls. I think in their current form, they are too specific to the current problem. What do you think about adding an “RPC calls sent” counter instead? 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 In case of an already existing cluster with reduced heartbeat interval, if batching is turned off, we should not alert for the default value. Batching can be turned on runtime. So we should warn then. Also you should be able to write: kudu-tserver --multi_raft_heartbeat_interval_ms=1500 --raft_heartbeat_interval_ms=3000 without getting an error. But validators run right when a flag is parsed, so it would run with raft_heartbeat_interval_ms still having its default value. http://gerrit.cloudera.org:8080/#/c/22867/8/src/kudu/consensus/multi_raft_batcher.cc@94 PS8, Line 94: Counter> no_o > nit: Counter Done 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 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: 9 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: Wed, 25 Jun 2025 20:24:12 +0000 Gerrit-HasComments: Yes
