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 17: (15 comments) http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.h File src/kudu/consensus/multi_raft_batcher.h: http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.h@90 PS15, Line 90: > nit: remove the stray period? Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.h@120 PS15, Line 120: std::deque<Callback> queue_; > Could you please add a comment to explain what this lock protects? Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.h@124 PS15, Line 124: std::mutex heartbeater_lock_; > style nit for here and elsewhere: move this into the very beginning of the Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.h@153 PS15, Line 153: ThreadPool* raft_p > Please document what this synchronization primitive protects. Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc File src/kudu/consensus/multi_raft_batcher.cc: http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@84 PS15, Line 84: uint64_t MultiRaftHeartbea > nit: remove this? Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@108 PS15, Line 108: window_(f > nit: wrap this into std::move() to avoid inc/dec of the usage counter? Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@116 PS15, Line 116: messenger_, > nit: add DCHECK() to protect against mistakes creating the timer multiple t Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@132 PS15, Line 132: KUDU_WARN_NOT_OK( : raft_pool_token_->Submit([this_ptr, current_calls = std::move(current_calls)]() { : this_ptr->SendOutScheduled(current_calls); : }), : "Failed to submit multi-raft heartbeat batcher task"); : current_calls.clear(); : current_calls.reserve(FLAGS_multi_raft_batch_size); : }; : : const auto sending_period = MonoDelta::FromMilliseconds(FLAGS_raft_heartbeat_interval_ms); : : std::lock_guard lock(heartbeater_lock_); : : auto send_until = MonoTime::Now() + batch_time_window_; : if (closed_) { > nit: is it possible to move this out of the critical section protected by t Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@171 PS15, Line 171: void MultiRaftHeartbeatBatcher::MultiRaftU > What is this for? left in debugging line. http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@185 PS15, Line 185: return; // > nit: could we release the heartbeater_lock_ right after this? Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_batcher.cc@275 PS15, Line 275: auto raft_pool_token = raft_pool_->NewToken(ThreadPool::ExecutionMode::CONCURRENT); : batcher = std::make_shared<MultiRaftHeartbeatBatcher>( : hostport, dns_resolver_, messenger_, batch_time_window_, move(raft_pool_token)); : batcher->StartTimer(); : batchers_[hostport] = batcher; : return batcher; : } : : void MultiRaftManager::Shutdown() { : s > Would it make sense to move the contents of batches_ and raft_pool_token_ ( I change it to have a raft pool token for each batcher. Since it is in concurrent mode, it does not matter at all in practice, but it feels more in line with the original "one token for a raft consensus", and there is no reason to have a shared one. http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_consensus_data.h File src/kudu/consensus/multi_raft_consensus_data.h: http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_consensus_data.h@50 PS15, Line 50: siz > nit: use size_t instead? Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/consensus/multi_raft_consensus_data.h@62 PS15, Line 62: } > Here might be dragons. The presence of this field (a) has some special sem Done http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/tserver/tablet_service.cc@1824 PS15, Line 1824: }; > nit: use auto here as well? We use "Status s =" everywhere else in the file too, except when we create one with the static functions of Status. http://gerrit.cloudera.org:8080/#/c/22867/15/src/kudu/tserver/tablet_service.cc@1830 PS15, Line 1830: } > nit: const auto& ? it is returned by value: "TabletStatePB state() const {" -- 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: 17 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: Fri, 22 Aug 2025 15:51:15 +0000 Gerrit-HasComments: Yes
