Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17097 )
Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager-test.cc File src/kudu/tablet/lock_manager-test.cc: PS5: Could you also add some (maybe multithreaded) tests for the Wait... functionality? http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.h File src/kudu/tablet/lock_manager.h: http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.h@81 PS5, Line 81: // Similar to the above, but wait until the lock is acquired. nit: could you mention that the caller is expected to ensure there is no deadlock? E.g. by only running on followers in the prepare phase, or running serially in an order that has already been successful with TryAcquirePartitionLock() calls? http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager.h File src/kudu/tablet/lock_manager.h: http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager.h@157 PS3, Line 157: multi-rows t > I found it is more clear to have a specific entity to represent the lock. E Yeah, if anything maybe we could call this PartitionLockState or something http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc File src/kudu/tablet/lock_manager.cc: http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc@367 PS5, Line 367: bool must_acquire nit: maybe use an enum? enum LockWaitMode { kWaitForLock, kTryLock, }; http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc@372 PS5, Line 372: DCHECK_NOTNULL(lock_); nit: can just used DCHECK(lock_) DCHECK_NOTNULL() returns the pointer, so this is creates an unused reference http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc@478 PS5, Line 478: (lock = TryAcquirePartitionLock(txn_id, &code)) Would it make sense to extend the TryAcquirePartitionLock() method with a timeout, similar to the semaphore at L537? I guess we'll see how frequently this codepath gets hit based on how frequently there are wait-die conflicts.. -- 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: 5 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: Thu, 11 Mar 2021 20:39:56 +0000 Gerrit-HasComments: Yes
