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] Add ThreadRestrictions-aware LockGuard and UniqueLock 

Hello David Ribeiro Alves, Alexey Serbin,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/7011

to review the following change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................

Add ThreadRestrictions-aware LockGuard and UniqueLock

These are thin wrapper classes around std::lock_guard and
std::unique_lock that automatically call
ThreadRestrictions::AssertWaitAllowed() when acquiring locks.

Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/lock_guard-test.cc
A src/kudu/util/lock_guard.h
3 files changed, 156 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7011/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
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] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................


Abandoned

Todd and Adar think these assertions should be in a Mutex implementation somewhere, and it seems the existence of this class could be confusing as an alternative to std::lock_guard/unique_lock, so I am dropping this patch.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
Gerrit-PatchSet: 4
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] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................


Patch Set 4:

> Also not sure I follow this. Typically these asserts would go in
 > the lock implementations themselves, rather than the guards, so
 > that you catch _all_ cases of waiting on the lock, not just the
 > ones that use our own custom wrapper.

Todd are you suggesting we add this to simple_spinlock or create a wrapper of simple_spinlock to do this instead?

I think that's more invasive but that would work too.

The main purpose of this patch is to "outsource" enforcement of these thread restrictions so we don't have to scatter them across a class when acquiring the lock.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
Gerrit-PatchSet: 4
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: No

[kudu-CR] Add ThreadRestrictions-aware LockGuard and UniqueLock

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/7011

to look at the new patch set (#4).

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................

Add ThreadRestrictions-aware LockGuard and UniqueLock

These are thin wrapper classes around std::lock_guard and
std::unique_lock that automatically call
ThreadRestrictions::AssertWaitAllowed() when acquiring locks.

Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/lock_guard-test.cc
A src/kudu/util/lock_guard.h
3 files changed, 167 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7011/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
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

[kudu-CR] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................


Patch Set 4:

Also not sure I follow this. Typically these asserts would go in the lock implementations themselves, rather than the guards, so that you catch _all_ cases of waiting on the lock, not just the ones that use our own custom wrapper.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7011/4//COMMIT_MSG
Commit Message:

PS4, Line 9: These are thin wrapper classes around std::lock_guard and
           : std::unique_lock that automatically call
           : ThreadRestrictions::AssertWaitAllowed() when acquiring locks.
It seems I'm missing some context here.

Why is it necessary to introduce such an automation?  I thought the idea was to deliberately enforce specifying those places where wait is permitted.

Is that about too many places where it would be necessary to do that if not introducing there wrappers?


http://gerrit.cloudera.org:8080/#/c/7011/4/src/kudu/util/lock_guard.h
File src/kudu/util/lock_guard.h:

PS4, Line 35:     ThreadRestrictions::AssertWaitAllowed();
            :   }
            : 
            :   ~LockGuard() {}
Does it make sense to restore the original disposition of the 'wait allowed' constraint in the destructor?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
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] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has uploaded a new patch set (#2).

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................

Add ThreadRestrictions-aware LockGuard and UniqueLock

These are thin wrapper classes around std::lock_guard and
std::unique_lock that automatically call
ThreadRestrictions::AssertWaitAllowed() when acquiring locks.

Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/lock_guard-test.cc
A src/kudu/util/lock_guard.h
3 files changed, 156 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7011/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
Gerrit-PatchSet: 2
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] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................


Patch Set 4:

> Also not sure I follow this. Typically these asserts would go in
 > the lock implementations themselves, rather than the guards, so
 > that you catch _all_ cases of waiting on the lock, not just the
 > ones that use our own custom wrapper.

Just to add one more thought: a lock guard like this would be used for acquiring all kinds of locks, but we should only call AssertWaitAllowed() when acquiring sleeping locks (i.e. not spinlocks).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
Gerrit-PatchSet: 4
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add ThreadRestrictions-aware LockGuard and UniqueLock

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7011/4/src/kudu/util/lock_guard.h
File src/kudu/util/lock_guard.h:

PS4, Line 35:     ThreadRestrictions::AssertWaitAllowed();
            :   }
            : 
            :   ~LockGuard() {}
> Does it make sense to restore the original disposition of the 'wait allowed
Oh, I misread this.  For some reason I read it as SetWaitAllowed(true).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
Gerrit-PatchSet: 4
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add ThreadRestrictions-aware LockGuard and UniqueLock

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/7011

to look at the new patch set (#3).

Change subject: Add ThreadRestrictions-aware LockGuard and UniqueLock
......................................................................

Add ThreadRestrictions-aware LockGuard and UniqueLock

These are thin wrapper classes around std::lock_guard and
std::unique_lock that automatically call
ThreadRestrictions::AssertWaitAllowed() when acquiring locks.

Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/lock_guard-test.cc
A src/kudu/util/lock_guard.h
3 files changed, 168 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/7011/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I87760f2721abab857165ad6e5ac6ac946992a927
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