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/08/31 08:38:52 UTC
[kudu-CR] Implement a lock table
Hello Dan Burkert, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7918
to review the following change.
Change subject: Implement a lock table
......................................................................
Implement a lock table
This lock table offers the abstraction of an unlimited number of
locks that can be used like reservations without holding a centralized
mutex.
An example of a use case where this is useful is an entry point where a
task is begun that may take some time while duplicate requests may
arrive. In such a case, a slot may be reserved in the lock table
corresponding to that task so that duplicate requests can be dropped
instead of serviced.
This initial implementation only offers a TryLock() interface. A
blocking Lock() method would be possible to efficiently implement in the
future, if needed, providing the underlying lock implementation was
changed from a spinlock to a mutex.
Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/lock_table-test.cc
A src/kudu/util/lock_table.h
3 files changed, 199 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/7918/1
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] Implement a lock table
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/7918
to look at the new patch set (#2).
Change subject: Implement a lock table
......................................................................
Implement a lock table
This lock table offers the abstraction of an unlimited number of
locks that can be used like reservations without holding a centralized
mutex.
An example of a use case where this is useful is an entry point where a
task is begun that may take some time while duplicate or conflicting
requests may arrive. In such a case, a slot may be reserved in the lock
table corresponding to that task so that duplicate or conflicting
requests can be rejected instead of serviced.
This initial implementation only offers a TryLock() interface. A
blocking Lock() method would be possible to efficiently implement in the
future, if needed, providing the underlying lock implementation was
changed from a spinlock to a mutex.
Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/lock_table-test.cc
A src/kudu/util/lock_table.h
3 files changed, 205 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/7918/2
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Implement a lock table
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Implement a lock table
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7918/1/src/kudu/util/lock_table.h
File src/kudu/util/lock_table.h:
Line 42: boost::optional<std::shared_ptr<LockTableToken<T>>> TryLock(T key) {
> Seems simpler to just return type std::shared_ptr<LockTableToken<T>>, with
Good idea.
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Implement a lock table
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Implement a lock table
......................................................................
Patch Set 2: Verified+1
Overriding unrelated flaky test failure of RaftConsensusITest.TestReplicaBehaviorViaRPC
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Implement a lock table
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change. ( http://gerrit.cloudera.org:8080/7918 )
Change subject: Implement a lock table
......................................................................
Abandoned
Turns out we didn't end up needing this so abandoning for now.
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-Change-Number: 7918
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Implement a lock table
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Implement a lock table
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Implement a lock table
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Implement a lock table
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7918/2/src/kudu/util/lock_table.h
File src/kudu/util/lock_table.h:
PS2, Line 40: T
I think calling this KEY or K would be clearer (at first wasn't sure what it was).
Also the docs should clarify what requirements there are of the 'T' type (eg naturally comparable with operator ==? must be hashable with std::hash? etc)
PS2, Line 46: std::shared_ptr<LockTableToken<T>>
hrm, why is this shared instead of just a move-only type combined with boost::optional<> to indicate failure
Line 48: auto result = slots_.insert(std::move(key));
how about using InsertIfNotPresent? and not doing the std::move here but rather down below when instantiating the token?
PS2, Line 52: this->s
'this->' is redundant right?
PS2, Line 69: slots_
to me the word 'slots' indicates that multiple entries may fall into the same 'slot' or something (ie that there are some fixed number of slots). Maybe just 'locks' or 'keys_' or something?
Line 98: const T& key() {
can be a const method
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Implement a lock table
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Implement a lock table
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7918/1/src/kudu/util/lock_table.h
File src/kudu/util/lock_table.h:
Line 42: boost::optional<std::shared_ptr<LockTableToken<T>>> TryLock(T key) {
Seems simpler to just return type std::shared_ptr<LockTableToken<T>>, with a nullptr for the failing case. They are equivalent in terms of safety.
--
To view, visit http://gerrit.cloudera.org:8080/7918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0404478675e079ba147fbdb4b0bac9db0cdc87e5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes