Adar Dembo has posted comments on this change.

Change subject: master: add read-write lock to serialize operations around 
elections
......................................................................


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3550/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS3, Line 345:  l.c
> could be more descriptive here?
Done


PS3, Line 367: aborting the current task...
> hmm, this (old) log message is nonsense, right?
Yeah. I'll just remove the whole thing. Not useful and adds noise to the 
control flow.


Line 657:     e.second->WaitTasksCompletion();
> does this put a restriction on the tasks themselves that they may not block
Correct. As it turns out, there was a separate deadlock issue which I've fixed: 
the task callbacks ran on the same singleton thread pool as this function. Now 
none of the tasks touch the leader lock.


http://gerrit.cloudera.org:8080/#/c/3550/3/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

PS3, Line 304: General status 
> a specific example of what types of "bad status" this might represent would
Done


PS3, Line 305: leadership_status
> you mean leader_status()?
Yes to both.


PS3, Line 308: Leadership status
> would a simple bool suffice?
CheckIsInitializedAndIsLeaderOrRespond() needs access to the status itself 
because it writes the message into the response, but that doesn't preclude the 
leader_status() accessor from being a bool.

But, this way it's more consistent with catalog_status(), which I like. Do you 
feel strongly?


PS3, Line 316: '.
> and returns false
Done


PS3, Line 691: etc
> what about read operations? should you specifically say write operations? a
As you observed, reads should also be guarded by this lock. It may be possible 
for reads to get by with the existing locks, but that means blocking for a long 
period of time on a spinlock, which isn't ideal. I went back and forth on this, 
but in the end reasoned that it's far simpler for _every_ operation to acquire 
it.


-- 
To view, visit http://gerrit.cloudera.org:8080/3550
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5084c09f1a77ccf620fb6cd621094c4778d636f8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to