[kudu-CR] (wip) KUDU-2612: acquire and release PartitionLock

2021-03-09 Thread Alexey Serbin (Code Review) 
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

2021-03-09 Thread Hao Hao (Code Review) 
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

2021-03-08 Thread Andrew Wong (Code Review) 
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

2021-03-08 Thread Hao Hao (Code Review) 
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