David Ribeiro Alves has posted comments on this change. Change subject: consensus: consolidate Raft thread pools ......................................................................
Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/6946/7/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: PS7, Line 133: shared_ptr<SequenceToken> locked_token = raft_pool_token_.lock(); : if (!locked_token) { : return Status::ServiceUnavailable("raft thread pool has been shutdown"); : } not sure I'm fully understanding this ownership model. Raft owns the token, right? , which isn't destroyed but on RaftConsensus dctor, at which point the peers should no longer exist. Thus it should be safe to pass the token by pointer or reference. Did you find any cases where that wasn't safe? EDIT: Oh I see, this is not the same token as in RaftConsensus. This token is shared between the queue and the peers. http://gerrit.cloudera.org:8080/#/c/6946/7/src/kudu/consensus/consensus_queue-test.cc File src/kudu/consensus/consensus_queue-test.cc: PS7, Line 20: #include <gtest/gtest.h> not your fault, but mind swapping this include with the one below? PS7, Line 45: using std::shared_ptr; remove? http://gerrit.cloudera.org:8080/#/c/6946/7/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS7, Line 197: shared_ptr<SequenceToken> raft_pool_token(std::make_shared<SequenceToken>( : raft_pool->AllocateTokenSlot())); hum, before this wasn't a single threaded TP and you're making it one, any reason for that? In particular, since this is shared between the peers and raft consensus there could be simultaneous things happening and now that isn't possible. -- To view, visit http://gerrit.cloudera.org:8080/6946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8958c607d11ac2e94908670c985059bef0ff5f54 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
