Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17159 )
Change subject: (wip) KUDU-2612: acquire and release PartitionLock ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/lock_manager.cc File src/kudu/tablet/lock_manager.cc: http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/lock_manager.cc@401 PS1, Line 401: void ScopedPartitionLock::TakeState(ScopedPartitionLock* other) { For consistency, maybe add a check that other != this ? http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/ops/participant_op.cc File src/kudu/tablet/ops/participant_op.cc: http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/ops/participant_op.cc@300 PS1, Line 300: state_->ReleaseTxn(); As for the sequence of releasing txn and its lock and partition lock: should the sequence of ReleaseTxn() and ReleasePartitionLock() be reversed? It seems these "locks" are acquired in the order of (1) txn lock (2) partition lock, so I'd expect those to be released in different order. Would it make sense? http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/ops/participant_op.cc@303 PS1, Line 303: const auto& op = state_->request()->op(); nit: could store the type of the operation instead since the operation as is isn't needed below? -- To view, visit http://gerrit.cloudera.org:8080/17159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 Gerrit-Change-Number: 17159 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Mar 2021 22:32:47 +0000 Gerrit-HasComments: Yes
