txn: Add batch-resumed mode for acquire_pessimistic_lock storage command #13687

MyonKeminta · 2022-10-28T06:58:11Z

What is changed and how it works?

Issue Number: Ref #13298

Requires: #13680

What's Changed:

Add batch-resumed mode for acquire_pessimistic_lock storage command.

Now the storage command `AcquirePessimisticLock` contains an enum to determine whether it's executing a normal request or it's a batch of requests resumed after waiting for another lock.

……but I doubt that whether it would be better if I add a new command instead of making AcquirePessimisticLock an enum inside

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

It will be covered by tests after another PR that makes use of it.

Side effects

Performance regression
- Consumes more CPU

Release note

None

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot · 2022-10-28T06:58:12Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
sticnarf

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…into m/support-lock-with-conflictt

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…lock-with-conflictt

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…on-of-pessimistic-lock-command

…med-version-of-pessimistic-lock-command

sticnarf · 2022-11-04T07:21:07Z

……but I doubt that whether it would be better if I add a new command instead of making AcquirePessimisticLock an enum inside

I think it looks better to add a new command. Is there any difficulty doing this way?

MyonKeminta · 2022-11-08T08:56:24Z

……but I doubt that whether it would be better if I add a new command instead of making AcquirePessimisticLock an enum inside

I think it looks better to add a new command. Is there any difficulty doing this way?

No. At first I was trying to reuse most of the code, but finally it looks totally different. I'll split it into two commands.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

sticnarf · 2022-11-09T09:03:17Z

src/storage/txn/commands/macros.rs

+    ($field: ident: enum_match { $( $arm: pat => $expr: expr ), * }) => {
+        fn gen_lock(&self) -> crate::storage::txn::latch::Lock {
+            match &self.$field {
+                $( $arm => crate::storage::txn::latch::Lock::new($expr) ),*
+            }
+        }
+    }


Where is this rule used?

Ah sorry. This is not used now and I forgot to delete it.

sticnarf · 2022-11-09T09:13:16Z

src/storage/txn/commands/acquire_pessimistic_lock_resumed.rs

+                if let Some(txn) = txn {
+                    if !txn.is_empty() {
+                        modifies.extend(txn.into_modifies());
+                    }
+                }
+                txn = Some(MvccTxn::new(
+                    params.start_ts,
+                    context.concurrency_manager.clone(),
+                ));


Suggested change

if let Some(txn) = txn {

if !txn.is_empty() {

modifies.extend(txn.into_modifies());

}

}

txn = Some(MvccTxn::new(

params.start_ts,

context.concurrency_manager.clone(),

));

if let Some(txn) = txn.replace(MvccTxn::new(...)) {

if !txn.is_empty() {

modifies.extend(txn.into_modifies());

}

}

replace is a bit simpler. Same for the reader below.

Is !txn.is_empty() necessary? IMO the code will look a bit simpler if removing the if.

sticnarf · 2022-11-09T09:20:29Z

src/storage/txn/commands/acquire_pessimistic_lock_resumed.rs

+                    if old_value.resolved() {
+                        let key = key.append_ts(txn.start_ts);
+                        // MutationType is unknown in AcquirePessimisticLock stage.
+                        let mutation_type = None;
+                        old_values.insert(key, (old_value, mutation_type));
+                    }


Seems we can extract this common code from acquire_pessimistic_lock, acquire_pessimistic_lock_resumed and prewrite.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

sticnarf · 2022-11-14T11:38:12Z

Please fix the conflict.

MyonKeminta · 2022-11-15T04:48:16Z

I'll fix conflicts after #13777 is merged.

…med-version-of-pessimistic-lock-command

ti-chi-bot · 2022-11-15T14:16:21Z

@TonsnakeLin: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

cfzjywxk

LGTM

src/storage/mod.rs

src/storage/txn/commands/acquire_pessimistic_lock_resumed.rs

MyonKeminta · 2022-11-16T09:00:01Z

/merge

ti-chi-bot · 2022-11-16T09:00:03Z

@MyonKeminta: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-11-16T09:00:05Z

This pull request has been accepted and is ready to merge.

Commit hash: d3f0931

…command

MyonKeminta added 4 commits October 27, 2022 15:04

Add lock-with-conflict support to acquire_pessimistic_lock

002b3e2

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add comments

98a42b7

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Remove commented code

e4e2bde

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add batch-resumed mode for acquire_pessimistic_lock storage command

b2a03b3

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot added do-not-merge/work-in-progress release-note-none size/XXL labels Oct 28, 2022

MyonKeminta added 3 commits November 1, 2022 17:36

Address comments

f412385

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'ft-pessimistic-lock-opt' of https://github.com/tikv/tikv …

f982809

…into m/support-lock-with-conflictt

Address comments

aeed936

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta changed the base branch from feature/pessimistic-lock-opt to master November 2, 2022 06:16

MyonKeminta added 6 commits November 2, 2022 15:58

Merge branch 'master' of https://github.com/tikv/tikv into m/support-…

d78359d

…lock-with-conflictt

Add more comments

a0eb137

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Address comments

95b0b39

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix typo

36ca004

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'm/support-lock-with-conflictt' into m/add-resumed-versi…

6a3c724

…on-of-pessimistic-lock-command

Merge branch 'master' of https://github.com/tikv/tikv into m/add-resu…

ad1fd4a

…med-version-of-pessimistic-lock-command

MyonKeminta marked this pull request as ready for review November 2, 2022 09:31

ti-chi-bot removed the do-not-merge/work-in-progress label Nov 2, 2022

MyonKeminta requested review from cfzjywxk, ekexium and sticnarf November 2, 2022 09:31

ti-chi-bot added the needs-rebase label Nov 4, 2022

Split into two commands

8118166

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot added size/XL and removed size/XXL labels Nov 9, 2022

misc updates

84bf650

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Address comments

0f4289f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

sticnarf reviewed Nov 9, 2022

View reviewed changes

MyonKeminta added 2 commits November 14, 2022 12:41

Address comments

851739a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add test for AcquirePessimisticLockResumed

03e59bc

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot added size/XXL and removed size/XL labels Nov 14, 2022

Address comments

5d68a3c

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'master' of https://github.com/tikv/tikv into m/add-resu…

d3f0931

…med-version-of-pessimistic-lock-command

ti-chi-bot removed the needs-rebase label Nov 15, 2022

MyonKeminta requested review from sticnarf and TonsnakeLin and removed request for TonsnakeLin November 15, 2022 06:35

sticnarf approved these changes Nov 15, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Status: PR - There is already 1 approval label Nov 15, 2022

TonsnakeLin approved these changes Nov 15, 2022

View reviewed changes

cfzjywxk approved these changes Nov 16, 2022

View reviewed changes

src/storage/mod.rs Show resolved Hide resolved

src/storage/txn/commands/acquire_pessimistic_lock_resumed.rs Show resolved Hide resolved

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Nov 16, 2022

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Nov 16, 2022

Merge branch 'master' into m/add-resumed-version-of-pessimistic-lock-…

b0a96fc

…command

ti-chi-bot merged commit 616b440 into tikv:master Nov 16, 2022

ti-chi-bot added this to the Pool milestone Nov 16, 2022

MyonKeminta mentioned this pull request Nov 21, 2022

Enhanced queueing for conflicting pessimistic transactions (Fair Locking) #13298

Open

30 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: Add batch-resumed mode for acquire_pessimistic_lock storage command #13687

txn: Add batch-resumed mode for acquire_pessimistic_lock storage command #13687

MyonKeminta commented Oct 28, 2022 •

edited

ti-chi-bot commented Oct 28, 2022 •

edited

sticnarf commented Nov 4, 2022

MyonKeminta commented Nov 8, 2022

sticnarf Nov 9, 2022

MyonKeminta Nov 9, 2022

sticnarf Nov 9, 2022

sticnarf Nov 9, 2022

sticnarf Nov 9, 2022

sticnarf Nov 9, 2022

sticnarf commented Nov 14, 2022

MyonKeminta commented Nov 15, 2022

ti-chi-bot commented Nov 15, 2022

cfzjywxk left a comment

MyonKeminta commented Nov 16, 2022

ti-chi-bot commented Nov 16, 2022

ti-chi-bot commented Nov 16, 2022

txn: Add batch-resumed mode for acquire_pessimistic_lock storage command #13687

txn: Add batch-resumed mode for acquire_pessimistic_lock storage command #13687

Conversation

MyonKeminta commented Oct 28, 2022 • edited

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Oct 28, 2022 • edited

sticnarf commented Nov 4, 2022

MyonKeminta commented Nov 8, 2022

sticnarf Nov 9, 2022

Choose a reason for hiding this comment

MyonKeminta Nov 9, 2022

Choose a reason for hiding this comment

sticnarf Nov 9, 2022

Choose a reason for hiding this comment

sticnarf Nov 9, 2022

Choose a reason for hiding this comment

sticnarf Nov 9, 2022

Choose a reason for hiding this comment

sticnarf Nov 9, 2022

Choose a reason for hiding this comment

sticnarf commented Nov 14, 2022

MyonKeminta commented Nov 15, 2022

ti-chi-bot commented Nov 15, 2022

cfzjywxk left a comment

Choose a reason for hiding this comment

MyonKeminta commented Nov 16, 2022

ti-chi-bot commented Nov 16, 2022

ti-chi-bot commented Nov 16, 2022

MyonKeminta commented Oct 28, 2022 •

edited

ti-chi-bot commented Oct 28, 2022 •

edited