You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/05/28 00:38:25 UTC
[kudu-CR] consensus: Get rid of LockFor*() methods
Hello David Ribeiro Alves, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7012
to review the following change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also:
* Use kudu::LockGuard and kudu::UniqueLock to enforce thread
restrictions.
* Add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 114 insertions(+), 192 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/1
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#9).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 159 insertions(+), 195 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/9
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/4/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS4, Line 117: State_Name
> style nit: why not just state_name() or StateName()?
State_Name() follows the convention used in protobuf which is used all over the place, so I thought it would be more intuitive, i.e. TabletStatePB_Name().
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#8).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 156 insertions(+), 191 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/8
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 12: Code-Review+2
(3 comments)
LGTM. You might be interested to get more feedback from Todd.
http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS10, Line 284: << State_Name(st
> Done
agreed -- it's better to make small verifiable steps
PS10, Line 2120: UniqueLock
> We need to unlock() below at L2178
woops, I missed that.
http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS10, Line 225: using UniqueLock = std::unique_lock<simple_spinlock>;
> see my reply on the other comment
Yep, thanks -- it seems I missed the unlock() part.
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#11).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 170 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/11
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
Line 1435: ThreadRestrictions::AssertWaitAllowed();
> Nah, we had them in the LockFor() calls.
My argument was more to do with our generic lock implementations; if RaftConsensus has its own spinlock with more restrictive assertions (and can guarantee that all of its threads meet those assertions), that's fine with me.
I think Todd's suggestion re: simple_spinlock is to subclass it as MaybeSlowLock and add the assertions there. Then it's easy to switch out the lock implementation from simple_spinlock to Mutex, and the generic spinlock implementation remains free of AssertWaitAllowed().
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 10:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS8, Line 718: wers is disabled. Doing nothing.";
> this message could be updated now
Done
Line 1435:
> seems like everywhere you take the lock, you are using AssertWaitAllowed.
I would rather do that in a follow-up patch since this patch is only intended to move code around without changing functionality. Right now we use a spinlock, not a mutex.
http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS10, Line 284: << "Replica is not in kInitialized state: "
> nit: I would drop this part since it does not make the message more actiona
Done
PS10, Line 313: }
:
: {
: ThreadRestrictions::AssertWaitAllowed();
: LockGuard l(lock_);
> It seems this could be safely removed.
I would rather do that in a follow-up patch since this patch is not intended to change any functionality.
PS10, Line 1360: SnoozeFailureDetectorUnlocked()
> What if it returns non-OK status? Should that case be handled somehow?
Nice catch, done.
PS10, Line 2120: UniqueLock
> why not LockGuard?
We need to unlock() below at L2178
Line 2178: lock.unlock();
We unlock here.
PS10, Line 2424: RETURN_NOT_OK(CheckActiveLeaderUnlocked());
: return Status::OK();
> nit: this could be reduced to
Done
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS8, Line 479:
> this should probably also have WARN_UNUSED_RESULT
Done
http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS10, Line 225: using UniqueLock = std::unique_lock<simple_spinlock>;
> I found only one place which uses the UniqueLock in the .cc file, and I'm n
see my reply on the other comment
PS10, Line 469: operation
> message?
I like the terminology of Operation better, since we are replicating things like Write operations. See OperationType in consensus.proto
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 10:
think you missed a couple comments on the previous rev?
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Reviewed-on: http://gerrit.cloudera.org:8080/7012
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 170 insertions(+), 207 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
Line 1435: ThreadRestrictions::AssertWaitAllowed();
> But you _have_ changed functionality by adding these AssertWaitAllowed call
Nah, we had them in the LockFor() calls.
Adar argued vociferously against putting this kind of check in a spinlock implementation on Slack, but I'm not against it.
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
Line 1435: ThreadRestrictions::AssertWaitAllowed();
> My argument was more to do with our generic lock implementations; if RaftCo
I agree that we can't put AssertWaitAllowed() into simple_spinlock itself for lots of reasons, which is why I didn't propose doing that. The conversation we had involved spinlocks in general as well and maybe there was a miscommunication there?
Anyway, I'll likely be switching this to a mutex in a follow-up and we can clean this up then.
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 4:
(1 comment)
it seems the AlterTableTest.TestAddRangePartitionConflictExhaustive deadlocked?
http://gerrit.cloudera.org:8080/#/c/7012/4/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS4, Line 117: State_Name
style nit: why not just state_name() or StateName()?
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/4/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS4, Line 310:
:
:
:
:
This looks like a place where the sequence of lock acquisitions and other actions has changed. The prior version released the lock for some reason: maybe, that's the root cause of the recent AlterTableTest.TestAddRangePartitionConflictExhaustive lock ups?
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 10:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS10, Line 284: << "Replica is not in kInitialized state: "
nit: I would drop this part since it does not make the message more actionable.
PS10, Line 313: }
:
: {
: ThreadRestrictions::AssertWaitAllowed();
: LockGuard l(lock_);
It seems this could be safely removed.
PS10, Line 1360: SnoozeFailureDetectorUnlocked()
What if it returns non-OK status? Should that case be handled somehow?
PS10, Line 2120: UniqueLock
why not LockGuard?
PS10, Line 2424: RETURN_NOT_OK(CheckActiveLeaderUnlocked());
: return Status::OK();
nit: this could be reduced to
return CheckActiveLeaderUnlocked();
http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS10, Line 225: using UniqueLock = std::unique_lock<simple_spinlock>;
I found only one place which uses the UniqueLock in the .cc file, and I'm not sure that's necessary. Consider dropping this.
PS10, Line 469: operation
message?
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#5).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 148 insertions(+), 194 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/5
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 10:
> think you missed a couple comments on the previous rev?
Ah, I missed your comments in the churn. Will take a look.
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#7).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 156 insertions(+), 191 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/7
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 4:
> (1 comment)
>
> it seems the AlterTableTest.TestAddRangePartitionConflictExhaustive
> deadlocked?
I think it's just flaky and it started last week, but I'm investigating. I did a git bisect over the weekend and I am about to verify the results
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#3).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also:
* Use kudu::LockGuard and kudu::UniqueLock to enforce thread
restrictions.
* Add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 112 insertions(+), 191 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/3
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#6).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 153 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/6
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
Line 1435: ThreadRestrictions::AssertWaitAllowed();
> Nah, we had them in the LockFor() calls.
Yea, Adar and I are both against putting it into simple_spinlock itself. However, I think making a subclass that's used here in this specific case, and putting them there is reasonable.
I forgot they were in the original calls. In that case I guess I'm OK removing them in a separate follow-on patch.
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#10).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 158 insertions(+), 195 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/10
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7012
to look at the new patch set (#12).
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
consensus: Get rid of LockFor*() methods
Simplify the locking logic by removing layers of abstraction.
Also add State_Name() helper for state-related error messages.
Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
3 files changed, 170 insertions(+), 207 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/12
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
Line 1435:
> I would rather do that in a follow-up patch since this patch is only intend
But you _have_ changed functionality by adding these AssertWaitAllowed calls everywhere, haven't you? we didn't use to have these assertions and now we do.
BTW the above code would work just as well with : public simple_spinlock, right?
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 10:
I looped raft_consensus-itest on dist-test 500x with 8 cpus of stress and the flaky tests looked basically the same as master under that kind of load. Here are the results:
http://dist-test.cloudera.org/job?job_id=mpercy.1496191228.26892
Failed tests:
16 RaftConsensusITest.TestMemoryRemainsConstantDespiteTwoDeadFollowers
7 RaftConsensusITest.TestCommitIndexFarBehindAfterLeaderElection
4 RaftConsensusITest.TestCorruptReplicaMetadata
2 RaftConsensusITest.TestReplicaBehaviorViaRPC
1 RaftConsensusITest.InsertUniqueKeysWithCrashyNodes
Crashed tests:
3 RaftConsensusITest.TestChurnyElections
1 RaftConsensusITest.InsertDuplicateKeysWithCrashyNodes
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Get rid of LockFor*() methods
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS8, Line 718: Unable to lock consensus for term change
this message could be updated now
Line 1435: ThreadRestrictions::AssertWaitAllowed();
seems like everywhere you take the lock, you are using AssertWaitAllowed.
Would it be easier to just do something like:
class MaybeSlowLock : public Mutex {
public:
void Lock() {
AssertWaitAllowed();
Mutex::Lock();
}
};
so that you don't have to do it everywhere?
http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS8, Line 479: Status CheckActiveLeaderUnlocked() const;
this should probably also have WARN_UNUSED_RESULT
--
To view, visit http://gerrit.cloudera.org:8080/7012
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes