[kudu-CR] (wip) KUDU-2612: acquire and release PartitionLock
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Mar 2021 22:32:47 + Gerrit-HasComments: Yes
[kudu-CR] (wip) KUDU-2612: acquire and release PartitionLock
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17159 to look at the new patch set (#2). Change subject: (wip) KUDU-2612: acquire and release PartitionLock .. (wip) KUDU-2612: acquire and release PartitionLock This patch introduces the logic for acquiring and releasing the PartitionLock at ParticipantOp for BEGIN_TXN, FINALIZE_COMMIT and ABORT_TXN. wip: move code for ScopedPartitionLock to the previous patch, add acquire and release PartitionLock at WriteOp, update TxnOpDispatcher error handling, add tests. Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 --- M src/kudu/tablet/lock_manager.cc M src/kudu/tablet/lock_manager.h M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/txn_participant-test.cc M src/kudu/tablet/txn_participant.h 8 files changed, 138 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/17159/2 -- 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: newpatchset Gerrit-Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 Gerrit-Change-Number: 17159 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] (wip) KUDU-2612: acquire and release PartitionLock
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17159 ) Change subject: (wip) KUDU-2612: acquire and release PartitionLock .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/17159/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17159/1//COMMIT_MSG@13 PS1, Line 13: wip: We'll also need to make sure the TxnOpDispatcher handles the error cases well 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@95 PS1, Line 95: else if (code == TabletServerErrorPB::TXN_LOCKED_RETRY_OP) { nit: based on the current code, this cannot be any other code, right? If so, maybe just use an else {} and DCHECK on the code? http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/ops/participant_op.cc@199 PS1, Line 199: state_->tablet_replica() nit: can use 'replica' http://gerrit.cloudera.org:8080/#/c/17159/1/src/kudu/tablet/ops/participant_op.cc@200 PS1, Line 200: case ParticipantOpPB::BEGIN_COMMIT: Shouldn't there be a 'break' before this line? Or are we taking the partition lock for each of these ops? -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 08 Mar 2021 19:52:49 + Gerrit-HasComments: Yes
[kudu-CR] (wip) KUDU-2612: acquire and release PartitionLock
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17159 Change subject: (wip) KUDU-2612: acquire and release PartitionLock .. (wip) KUDU-2612: acquire and release PartitionLock This patch introduces the logic for acquiring and releasing the PartitionLock at ParticipantOp for BEGIN_TXN, FINALIZE_COMMIT and ABORT_TXN. wip: move code for ScopedPartitionLock to the previous patch, add acquire and release PartitionLock at WriteOp, add tests. Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 --- M src/kudu/tablet/lock_manager.cc M src/kudu/tablet/lock_manager.h M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 6 files changed, 91 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/17159/1 -- 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: newchange Gerrit-Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 Gerrit-Change-Number: 17159 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao