Adar Dembo has posted comments on this change. Change subject: consensus: consolidate Raft thread pools ......................................................................
Patch Set 7: (4 comments) I filed KUDU-2029 for the unrelated build failure. 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, Yes, this token is shared between RaftConsensus and the peers, and this is the token used by RaftConsensus itself. However, RaftConsensus is the owner: the token should be destroyed when RaftConsensus is destroyed. Using weak_ptr here instead of a shared_ptr enforces that. Given the RaftConsensus lifecycle I could have shared the token as a raw pointer, but I think "shared_ptr for the owner, weak_ptr for dependents" better describes the relationship, and it's safer too. In general I think we should use this pattern instead of the "unique_ptr for the owner, raw pointers for dependents" style that's more prevalent in Kudu today. The status quo is from a time before we had access to C++11's weak pointers, and it's less expressive and less safe. The only exception should be if creating/destroying dependents is a hot path and we don't want to incur the overhead of weak ref count add/dec. 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? Done PS7, Line 45: using std::shared_ptr; > remove? Done 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 Right, the use of a token here is so that we can call SequenceToken::Shutdown() during RaftConsensus::Shutdown() and thus ensure that there are no outstanding tasks when RaftConsensus is finished shutting down. Those are the semantics we had when we used a dedicated threadpool. If you think the serialization aspect of the token is bad, I can think of a couple options: 1. Remove the call to SequenceToken::Shutdown() from RaftConsensus::Shutdown() and restore the "last task can destroy its own token" logic that I removed (didn't need it if we shutdown tokens). 2. Expand the token logic to allow for the allocation of "non-serialized" tokens. They would still allow for the logical grouping of tasks (and, as a result, token shutdown), but their tasks could be processed in parallel by worker threads. I think this can be done but I haven't really thought through the complexity. -- 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
