Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17097 )
Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock ...................................................................... Patch Set 2: (18 comments) http://gerrit.cloudera.org:8080/#/c/17097/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17097/2//COMMIT_MSG@17 PS2, Line 17: transaction nit: you meant a write operation here, not the whole transaction, right? 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@223 PS2, Line 223: } nit: are there any post-conditions to check after releasing all the row locks? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h File src/kudu/tablet/lock_manager.h: http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@46 PS2, Line 46: // Super-simple lock manager implementation. This only supports exclusive : // locks, and makes no attempt to prevent deadlocks if a single thread : // takes multiple locks. : // : // In the future when we want to support multi-row transactions of some kind : // we'll have to implement a proper lock manager with all its trappings, : // but this should be enough for the single-row use case. nit: maybe, update this to reflect changes in this patch? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@85 PS2, Line 85: // Lock to protect 'partition_lock_' and : // 'partition_lock_refs_'. nit: might be at a single line http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@90 PS2, Line 90: by : // the same transaction. nit: do you mind extending the comment to explain where the information to identify that 'the same' transaction is stored? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@154 PS2, Line 154: class PartitionLock { nit: mind adding a short description for this class? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@181 PS2, Line 181: assignment nit: assignment operator http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@188 PS2, Line 188: Acquired style nit: rename to 'IsAcquired' or 'is_acquired' http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.h@189 PS2, Line 189: Release nit: add a doc? 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@365 PS2, Line 365: LockManager::LockMode mode, I could not find where this parameter is used. Did I miss anything? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@369 PS2, Line 369: DCHECK_EQ(LockManager::LOCK_EXCLUSIVE, mode); If this should always be LockManager::LOCK_EXCLUSIVE, why not to drop the 'mode' parameter at all? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@390 PS2, Line 390: manager_->ReleasePartitionLock(); Does 'lock_' needs to be updated after calling ReleasePartitionLock() ? 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 gau +1 BTW, what about non-rvalue constructor and assignment operator? Is it necessary to override them as well? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@415 PS2, Line 415: () nit: the parentheses are not required here http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@425 PS2, Line 425: PREDICT_TRUE Do we expect this to be used by non-transactional operations as well? If so, is my reading correct that we expect more transactional writes than non-transactional writes? If so, why? If not, maybe remove the PREDICT_TRUE hint? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@434 PS2, Line 434: if (PREDICT_FALSE(txn_id.value() > partition_lock_->txn_id().value())) { : *code = tserver::TabletServerErrorPB::TXN_LOCKED_ABORT; : return nullptr; : } : if (PREDICT_FALSE(txn_id.value() < partition_lock_->txn_id().value())) { : *code = tserver::TabletServerErrorPB::TXN_LOCKED_RETRY; : return nullptr; : } Looking at this code makes me think that the most anticipated case if when the same transaction tries to acquire the same lock. Is that so indeed? If not, maybe remove those PREDICT_FALSE() hints? http://gerrit.cloudera.org:8080/#/c/17097/2/src/kudu/tablet/lock_manager.cc@449 PS2, Line 449: } else { : return nullptr; : } readability nit: put this condition first and remove the extra 'else' clause 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@118 PS2, Line 118: TXN_LOCKED_RETRY nit: maybe, rename into TXN_LOCKED_RETRY_OP to signify that it's an operation, not whole transaction needs to be retried? -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 22 Feb 2021 17:33:35 +0000 Gerrit-HasComments: Yes
