Adar Dembo has posted comments on this change. Change subject: consensus: consolidate Raft thread pools ......................................................................
Patch Set 7: (1 comment) 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())); > We need the token shutdown logic to be able to clear the queue's observers, Quick correction: the observers are on their own dedicated token (see L188). That use case _does_ need Shutdown, as you described. But, I don't think the RaftConsensus token needs Shutdown(), because it's got that funky lifecycle wherein it binds (and refs) itself into each submitted task. Thus, I don't believe either ThreadPool::Shutdown() (before my patch) or SequenceToken::Shutdown() (after my patch) are needed in RaftConsensus::Shutdown(). At best they may prevent tasks from being submitted after RaftConsensus has shut down, but eventually no more tasks will be submitted, and the last task will destroy RaftConsensus when it finishes running. Having said all that, I gave it some more thought and option 1 doesn't actually solve the problem. Yes, it does away with the call to SequenceToken::Shutdown(), but both RaftConsensus and Peer tasks are still being submitted through one token and thus being serialized. I'll investigate the feasibility of "non-serialized" tokens and report back with my findings. -- 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
