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 <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
