Alexey Serbin has posted comments on this change. Change subject: [catalog manager] fixed deadlock on catalog shutdown ......................................................................
Patch Set 6: (6 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 Done 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. Well, this looks ugly but it works. I don't think it's not worth changing in this patch. Maybe, we can do it in a separate changelist. Line 803: const Consensus* consensus = sys_catalog_->tablet_peer()->consensus(); > If the concern is use-after free then would this work? There isn't a problem with use-after free here. That's guaranteed by the shutdown sequence implemented in the CatalogManageer::Shutdown() method. In essence, the code awaits for this task to complete before resetting the tablet peer. Line 803: const Consensus* consensus = sys_catalog_->tablet_peer()->consensus(); > I should have said something like: if (!tablet_peer) return; yup. 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; > Agreed, I don't think we can prohibit callers from invoking this when not r ok, I'll change the signature. But let's do that in a separate changelist. Line 1033: // may be destroyed while still in use by the ElectedAsLeaderCb task. > I added a suggestion to fix this above I see, but that would be much laborious and invasive change. I tried that route as the first approach, and after discussion with Adar and some consideration I came up with the current one. -- 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
