David Ribeiro Alves 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())); > Right, the use of a token here is so that we can call SequenceToken::Shutdo We need the token shutdown logic to be able to clear the queue's observers, that much we had discovered when we last discussed this. Not sure whether the final Wait() that you had on consensus shutdown was safe. For raft itself and the peers if feels forced to have to choose total serialization to be able to shutdown the tasks. In this case it's not too bad, raft actually serializes most calls under a single lock, as does the queue meaning there could be at most two things happening at the same time, at least for the "real work" paths. However I would expect this not to be the common case, so Ideally, I think your generalization of the token logic to multiple threads. I.e. basically have a "context" within a TP that you can shutdown and dictate how many threads it can have and grow to within a larger TP from which you pull threads for efficient would be preferable and the most powerful solution. If you feel like the TP saga has been going long enough another, likely easier, thing we could do it to just have two sequence tokens, one for the peers and one for raft. -- 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
