Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17097 )
Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager-test.cc File src/kudu/tablet/lock_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager-test.cc@159 PS2, Line 159: TEST_F(LockManagerTest, TestLockUnlockPartitionMultiTxn) { : // Acquiring a lock that held by another transaction which has a lower txn ID : // will get a 'TXN_LOCKED_ABORT' server error code. : { : TxnId txn1(0); : ScopedPartitionLock first_lock(&lock_manager_, LockManager::LOCK_EXCLUSIVE, txn1); : TabletServerErrorPB::Code code = TabletServerErrorPB::UNKNOWN_ERROR; : ASSERT_TRUE(first_lock.Acquired(&code)); : ASSERT_EQ(TabletServerErrorPB::UNKNOWN_ERROR, code); : ASSERT_EQ(1, lock_manager_.partition_lock_refs()); : : TxnId txn2(1); : ScopedPartitionLock second_lock(&lock_manager_, LockManager::LOCK_EXCLUSIVE, txn2); : ASSERT_FALSE(second_lock.Acquired(&code)); : ASSERT_EQ(TabletServerErrorPB::TXN_LOCKED_ABORT, code); : } nit: probably makes sense to split TXN_LOCKED_ABORT code testing into a separate test case. http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager-test.cc@179 PS2, Line 179: TxnId txn1(1); : ScopedPartitionLock first_lock(&lock_manager_, LockManager::LOCK_EXCLUSIVE, txn1); : TabletServerErrorPB::Code code = TabletServerErrorPB::UNKNOWN_ERROR; : ASSERT_TRUE(first_lock.Acquired(&code)); : ASSERT_EQ(TabletServerErrorPB::UNKNOWN_ERROR, code); : ASSERT_EQ(1, lock_manager_.partition_lock_refs()); : : TxnId txn2(0); nit: the values of these transaction IDs are pretty confusing IMO. Might be easier to reason about if txn1 has value 1, txn2 has value 2, etc. Or at the very least, maintain ordering? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc File src/kudu/tablet/lock_manager.cc: http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@368 PS2, Line 368: code_ Why bother storing this at all if we evaluate it every time we create a ScopedPartitionLock? Why not just call AcquirePartitionLock() when we call Acquired()? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@392 PS2, Line 392: : ScopedPartitionLock::ScopedPartitionLock(ScopedPartitionLock&& other) noexcept { : TakeState(&other); : } : : ScopedPartitionLock& ScopedPartitionLock::operator=(ScopedPartitionLock&& other) noexcept { : TakeState(&other); : return *this; : } Curious, how do we expect to use these? It isn't clear, but that'd help gauge the correctness of TakeState(). http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@422 PS2, Line 422: PartitionLock* LockManager::AcquirePartitionLock( : const TxnId& txn_id, : TabletServerErrorPB::Code* code) { What happens if a non-transaction attempts to take the lock while a transaction is on-going? Can you add a test for when this is called with an invalid txn ID, and what happens if that races with a non-invalid txn ID? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tserver/tserver.proto@116 PS2, Line 116: transaction nit: to be clear, the whole transaction doesn't need to be retried, just the write op, right? -- 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: 2 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 22 Feb 2021 06:30:24 +0000 Gerrit-HasComments: Yes
