Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17097 )
Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager-test.cc File src/kudu/tablet/lock_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager-test.cc@158 PS6, Line 158: ASSERT_EQ(1, lock_manager_.partition_lock_refs()); Looking at this as a block box, does it make sense to check that the first lock is still held even if the other went out of the scope? http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager-test.cc@252 PS6, Line 252: if (acquired) { Why it might not be acquired? Is it possible? http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager-test.cc@267 PS6, Line 267: if (acquired) { : CHECK_EQ(TabletServerErrorPB::UNKNOWN_ERROR, code); : CHECK_EQ(1, lock_manager_.partition_lock_refs()); : } I'm not sure I understand this piece if reading the comment for the thread's functor: it seems the lock should not be ever acquired. If so, why to check for acquired at all? http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.h File src/kudu/tablet/lock_manager.h: http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.h@193 PS6, Line 193: ScopedPartitionLock What if an instance of this class is copied? Would it result in incorrect lock reference count for the underlying PartitionLock? http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.cc File src/kudu/tablet/lock_manager.cc: http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.cc@448 PS6, Line 448: kMaxBackoffExp nit: this might be a constexpr http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.cc@485 PS6, Line 485: SleepFor(MonoDelta::FromMilliseconds(1L << backoff_exp)); > It's bad practice to sleep in a thread that is holding a lock. Is there any BTW, why to hold that spinlock-based lock when sleeping (i.e., why not to release it before going to sleep) -- it's possible to re-acquire it on the next iteration of the loop. Anywas, I guess using Semaphore::TimedAcquire() might be the best option as Andrew suggested. http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.cc@496 PS6, Line 496: ms Is it in seconds or milliseconds? I guess the report might be skewed because thread might be put to sleep for longer. Would it make sense just to use KLOG_EVERY_N_SECS() here instead, so the wait interval passed to TryAcquirePartitionLock() could be arbitrary? http://gerrit.cloudera.org:8080/#/c/17097/6/src/kudu/tablet/lock_manager.cc@520 PS6, Line 520: partition_lock_refs_ -= 1; : DCHECK_GE(partition_lock_refs_, 0); : if (partition_lock_refs_ == 0) { : partition_lock_.reset(); : } nit: this might be shortened to DCHECK_GT(partition_lock_refs_, 0); if (--partition_lock_refs == 0) { partition_lock_.reset(); } -- To view, visit http://gerrit.cloudera.org:8080/17097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I158115739ce3e7cfb77bbcb854e834336c1256b1 Gerrit-Change-Number: 17097 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 12 Mar 2021 03:51:58 +0000 Gerrit-HasComments: Yes
