Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17097 )
Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock ...................................................................... Patch Set 5: (9 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... functio Ah right, missed that. http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager-test.cc@261 PS5, Line 261: partition_lock.IsAcquired(&code) > Ah, I guess you can use the moved instances, but maybe add specific NOLINT Ack 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 de Done 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? Done 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_) Done http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc@409 PS5, Line 409: manager_ = other->manager_; > nit: add a DCHECK() on other != this, just to catch programming errors. Done http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc@437 PS5, Line 437: if (!txn_id.IsValid()) { : id = std::numeric_limits<int64_t>::max(); : } else { : id = txn_id.value(); : } > nit: this might be done using tri-state operator: Done http://gerrit.cloudera.org:8080/#/c/17097/5/src/kudu/tablet/lock_manager.cc@473 PS5, Line 473: PartitionLock* LockManager::WaitUntilAcquiredPartitionLock(const TxnId& txn_id) { > What happens when this is running for too long, going over the overall time Yeah, this is used for case where we expect the lock can be acquired eventually (e.g. for a follower to replicate the participanOp). 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 t Makes sense, updated it. -- 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 <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 12 Mar 2021 00:18:21 +0000 Gerrit-HasComments: Yes
