Adar Dembo has posted comments on this change.
Change subject: master: add read-write lock to serialize operations around
Patch Set 3:
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
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.
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
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 http://gerrit.cloudera.org:8080/3550
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
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>