Adar Dembo has posted comments on this change.

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

Patch Set 3:

File src/kudu/master/

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

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.
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

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

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 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5084c09f1a77ccf620fb6cd621094c4778d636f8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to