Mike Percy has posted comments on this change. Change subject: [catalog manager] fixed deadlock on catalog shutdown ......................................................................
Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/6134/6//COMMIT_MSG Commit Message: Line 16: Prior to the fix, the deadlock happened pretty often while running Would be good to mention if you tested this on dist-test to ensure the test isn't flaky anymore. http://gerrit.cloudera.org:8080/#/c/6134/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 801: shared_lock<LockType> l(lock_); Uh... what the heck. Line 803: const Consensus* consensus = sys_catalog_->tablet_peer()->consensus(); If the concern is use-after free then would this work? scoped_refptr<TabletPeer> tablet_peer = ys_catalog_->tablet_peer(); Consensus* c = tablet_peer->consensus(); ... PS6, Line 976: std::lock_guard<simple_spinlock> l(state_lock_); : DCHECK_EQ(kRunning, state_); : if (state_ == kRunning) { : return sys_catalog_->tablet_peer()->consensus()->role(); : } : return RaftPeerPB::UNKNOWN_ROLE; > this shouldn't likely DCHECK since it might be called on init/shutdown. Agreed, I don't think we can prohibit callers from invoking this when not running because that is inherently racy. We could also just change the signature to return Status::InvalidState if we're not running and take Role as a out-param pointer. -- To view, visit http://gerrit.cloudera.org:8080/6134 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10ad66fe33d4696adf2a02a09e2790afa8869583 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
