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

Reply via email to