Alexey Serbin 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: (9 comments) http://gerrit.cloudera.org:8080/#/c/22867/5//COMMIT_MSG Commit Message: PS5: > So far, I’ve tested it by setting enable_multi_raft_heartbeat_batcher=1 and Yes, that makes sense. It saw there was some feedback from a few reviewers already, and I'll need to take a closer look at this soon as well. http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@20 PS9, Line 20: allows the following nit: allows for the following http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@35 PS9, Line 35: 1 master + 4 TS Were those RELEASE or DEBUG binaries? I'd guess those were RELEASE ones, right? http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@37 PS9, Line 37: performed the following random sampling Did you sample that only for a particular tablet server or that's the stats across all four tablet servers in the cluster? http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@39 PS9, Line 39: the flag nit: which exact flag? would be great to mention http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@40 PS9, Line 40: CPU usage and packet count BTW, did you happen to look at the locking primitives usage? Anything like 'perf lock' https://www.man7.org/linux/man-pages/man1/perf-lock.1.html and similar? I'm also curious to see numbers for scheduler events, if possible to compare numbers for idle tablet servers with and without empty Raft heartbeats batching: perf stat -e 'sched:sched_*' http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@44 PS9, Line 44: write time call time nit: seems to be a duplication? http://gerrit.cloudera.org:8080/#/c/22867/9//COMMIT_MSG@47 PS9, Line 47: nit for here and below: replace tabs with spaces for consistent formatting 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@422 PS9, Line 422: required There was an intent to switch to protobuf3 eventually, and having 'required' fields would make the transition harder. Is it possible to have these fields 'optional' in these new protobuf messages or it will require changing the existing code to check for the presence of fields if doing so? -- 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: Tue, 01 Jul 2025 07:07:46 +0000 Gerrit-HasComments: Yes
