Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22867 )
Change subject: Add option to send no-op heartbeat operations batched PART1 ...................................................................... Patch Set 5: (14 comments) I took a quick first look, posted a few high-level comments regarding the structure of the patch, rather than its essence. I'm planning to take a deeper look this week. http://gerrit.cloudera.org:8080/#/c/22867/5//COMMIT_MSG Commit Message: PS5: I'd expect to see at least a few (unit?) tests for MultiRaftHeartbeatBatcher in this patch, but I didn't find any. Does it mean the batcher isn't yet functional or you just forgot to do 'git add' for the corresponding files when publishing this patch, or that's something else? http://gerrit.cloudera.org:8080/#/c/22867/5//COMMIT_MSG@7 PS5, Line 7: Add option to send no-op heartbeat operations batched PART1 Consider opening a JIRA item at https://issues.apache.org/jira for tracking this. Let me know if you need help with file a new JIRA item. http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/client/client-test.cc@194 PS5, Line 194: DECLARE_int32(multi_raft_batch_size); : DECLARE_bool(enable_multi_raft_heartbeat_batcher); Are these really needed here? http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/client/client-test.cc@10357 PS5, Line 10357: ASSERT_EVENTUALLY Consider separating this update on its own patch. Also, please add a comment explaining why it's necessary to wrap the code below into ASSERT_EVENTUALLY block. http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/client/client-test.cc@10364 PS5, Line 10364: sort(row.begin(), row.end()); Why do we need sort here? If it's really necessary, please add a comment to explain. http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/consensus.proto@419 PS5, Line 419: tablet_id Is 'tablet_id' really shared between messages? http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/consensus.proto@438 PS5, Line 438: ConsensusRequestPB Is this a typo? http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/consensus_peers.h File src/kudu/consensus/consensus_peers.h: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/consensus_peers.h@232 PS5, Line 232: NO_ACTIVE, : PENDING_NON_BUFFERED, : PENDING_BUFFERED_PREPARE, : PENDING_BUFFERED_POSSIBLY_IN_BUFFERED, : PENDING_BUFFERED_REQUEST_TO_FLUSH, : PENDING_BUFFERED_FLUSHED Please add short description for each of the enumerators. http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/consensus_peers.h@240 PS5, Line 240: std::uint64_t nit: is it possible to have just 'uint64_t'? http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/multi_raft_batcher.h File src/kudu/consensus/multi_raft_batcher.h: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/multi_raft_batcher.h@18 PS5, Line 18: #ifndef KUDU_MULTI_RAFT_BATCHER_H : #define KUDU_MULTI_RAFT_BATCHER_H style nit: in new code, please use '#pragma once' instead of include guard macro http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/consensus/multi_raft_batcher.h@104 PS5, Line 104: std::shared_ptr<rpc::PeriodicTimer> batch_sender_; : : std::mutex mutex_; : : std::shared_ptr<MultiRaftConsensusData> current_batch_; : : std::atomic<uint64_t> buffer_start_idx; Please document all these non-trivial fields. http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/master/sys_catalog.cc@485 PS5, Line 485: multi_raft_manager_ = std::make_unique<consensus::MultiRaftManager>(master_->messenger(), : master_->dns_resolver(), : master_->metric_entity()); Why to create this instance here, but on in the constructor? Please add a comment explaining why. http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/tserver/tablet_service.cc@1843 PS5, Line 1843: mzzzzz nit for here and elsewhere: consider using TODO(github_name) instead for these http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/22867/5/src/kudu/tserver/ts_tablet_manager.cc@520 PS5, Line 520: multi_raft_manager_ = std::make_unique<consensus::MultiRaftManager>( : server_->messenger(), server_->dns_resolver(), server_->metric_entity()); What makes it impossible to create this instance in the constructor? Please add a small comment explaining why. -- 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: 5 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: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 02 Jun 2025 21:41:20 +0000 Gerrit-HasComments: Yes
