Henry Robinson has posted comments on this change. Change subject: IMPALA-4456: Change query_exec_state_lock_ to a reader-writer lock ......................................................................
Patch Set 3: (4 comments) Did you consider boost::scoped_mutex? http://gerrit.cloudera.org:8080/#/c/5021/2/be/src/gutil/read_write_lock.cc File be/src/gutil/read_write_lock.cc: PS2, Line 2: // Use of this source code is governed by a BSD-style license that can be : // found in the LICENSE file. : : // This file has had some code modified and removed from the original version. Same comments as header. http://gerrit.cloudera.org:8080/#/c/5021/2/be/src/gutil/read_write_lock.h File be/src/gutil/read_write_lock.h: PS2, Line 3: LICENSE Remove this sentence, I think, because it's covered by LICENSE.txt (but please check that the LICENSE referred to here is the same as that in LICENSE.txt for util/). PS2, Line 5: This file has had some code modified and removed from the original version By whom - by us? Or was this line present when you copied the file? http://gerrit.cloudera.org:8080/#/c/5021/2/be/src/util/rw_lock.h File be/src/util/rw_lock.h: PS2, Line 25: class ReadWriteLock { : public: : ReadWriteLock() {} : : // Reader lock functions. : void ReadAcquire() { l.ReadAcquire(); } : void ReadRelease() { l.ReadRelease(); } : : // Writer lock functions. : void WriteAcquire() { l.WriteAcquire(); } : void WriteRelease() { l.WriteRelease(); } : : private: : base::subtle::ReadWriteLock l; : : DISALLOW_COPY_AND_ASSIGN(ReadWriteLock); : }; > I noticed that we don't have the habit of using objects from other namespac I think that's just because the majority of support classes we use come from boost or std. We use gutil's classes where appropriate - that's why we have it in the codebase! So this wrapper seems unnecessary to me. -- To view, visit http://gerrit.cloudera.org:8080/5021 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I790a95e7179f07aa7ba188d5422c5e054353ba0b Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
