Adar Dembo has posted comments on this change. Change subject: consensus: consolidate Raft thread pools ......................................................................
Patch Set 6: David and I looked at an interesting TSAN failure together (https://gist.github.com/adembo/dc2122e99468523bc8dafec8ad5a62d8) and I wanted to publish our findings. The change in semantics to PeerMessageQueue::Close() is unsafe. Previously, the observers pool was exclusively owned by RaftConsensus, and shutting it down before closing the queue ensured that any threads handling peer responses concurrently would be unable to submit tasks to the observers pool. Replacing ThreadPool::Shutdown() with SequenceToken::Wait() does not prevent future submissions to the pool, which is a problem because submitted tasks do not take refs on PeerMessageQueue or RaftConsensus (nor should they; after all, PeerMessageQueue is designed to be outlived by RaftConsensus), and so by the time they run, RaftConsensus may have been destroyed. David and I considered various ways to solve this problem from within PeerMessageQueue itself, but they all seemed complex. One approach is to reacquire the queue's lock before submitting to the token, checking if the queue has been closed first. Unfortunately this would require holding the queue spinlock while grabbing the pool's mutex, which doesn't seem like a good idea. Instead, I think I'm going to bite the bullet and provide a SequenceToken::Shutdown() method that would atomically stop future submissions on the token and wait for current submissions to finish. We worked out a rough way to implement this using an additional SequenceSlot state, without the need for an extra lock. I'll give it a shot next week. -- 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: 6 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: No
