David Ribeiro Alves has posted comments on this change. Change subject: [catalog manager] fixed deadlock on catalog shutdown ......................................................................
Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/6134/4//COMMIT_MSG Commit Message: PS4, Line 7: [catalog manager] fixed deadlock on catalog shutdown > There is no stack trace on the deadlock: it's not a deadlock on synchroniza well, you could always pstack the daemon and see where the threads were stuck, but that's ok now. http://gerrit.cloudera.org:8080/#/c/6134/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS4, Line 1022: AbortAndWaitForAllTasks(copy); > I think David meant sys_catalog_, not sync_catalog_. yes, adar got it right. sorry about the confusion PS4, Line 1024: consensus > This extremely confusing. Why then such a name for the member variable? :) oh, you're right. My bad here. We so refer to "consensus implementation" or "consensus algorithm" when talking about changes or queries to the consensus implementation running on the local machine and "raft config" or "consensus config" when talking about the set of machines/daemons running consensus for a given tablet. apologies for the misdirect PS4, Line 1030: Raft quorum > Why 'configuration'? The message here is that other members of the Raft qu this discussion came up a few times. Initially when I implemented it I used to refer to "consensus quorum" or "raft quorum" all the time. The point against that, as it was pointed out, is that is form of "synecdoche": quorum refers to a majority of peer and not to the whole set. http://gerrit.cloudera.org:8080/#/c/6134/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS6, Line 976: } : sys_catalog_.reset(new_catalog.release()); : return Status::OK(); : } : : bool CatalogManager::IsInitialized this shouldn't likely DCHECK since it might be called on init/shutdown. Also to avoid any lock inversion problem or corner case we should likely do something like: scoped_refptr<Consensus> consensus; { std::lock_guard<simple_spinlock> l(state_lock_); if (state_ == kRunning) { consensus = sys_catalog_->tablet_peer()->shared_consensus(); } else { return RaftPeerPB::UNKNOWN_ROLE; } } return consensus->role(); -- 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: 4 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
