server/lock_manager: Support reporting the full wait chain when deadlock is detected by MyonKeminta · Pull Request #10085 · tikv/tikv

MyonKeminta · 2021-04-26T13:26:57Z

~~This PR is now reviewable. But before merging, I want to test if there's any performance regression and try to optimize it if necessary.~~

What problem does this PR solve?

Problem Summary:

We are going to implement the feature Lock View . A part of it is to support collecting information about recent deadlocks.

What is changed and how it works?

What's Changed: Two more fields key and resource_group_tag is added to deadlock detection request, deadlock detector's inner information, and deadlock error message. SQL Digest will be encoded into resource_group_tag. The resource_group_tag may also be used by another feature Top SQL, and they want to pass both SQL Digest and the plan to TiKV by the resource_group_tag.

Related changes

Requires kvproto: Add proto for implementing lock view pingcap/kvproto#752

Check List

Tests

Unit test
Manual test
- Tested several simple workload with this PR: executor, store: Pass the SQL digest down to pessimistic lock request pingcap/tidb#24380 , and compared with the merge base to master.
- There are 1 TiDB node and 3 TiKV nodes, each has 16c cpu adn 90G mem (TiDB has 64G), nvme disk.
- No observable performance regression is found, no observable difference is found in TiKV's metrics.

With this PR	Workload	Threads	Table size	QPS	.95 lat
Y	oltp_read_write	32	100,000	14455.2	54.83
Y	oltp_read_write	128	100,000	24456.78	127.81
	oltp_read_write	32	100,000	14522.35	54.83
	oltp_read_write	128	100,000	24734.54	127.81
Y	oltp_write_only	32	100,000	10184.40	31.37
Y	oltp_write_only	128	100,000	21315.33	80.03
	oltp_write_only	32	100,000	10227.46	31.37
	oltp_write_only	128	100,000	20600.07	82.96

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM

Release note

No release note

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

MyonKeminta · 2021-04-27T06:51:05Z

/bench

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

MyonKeminta · 2021-04-27T14:10:51Z

/release

sre-bot · 2021-04-27T14:35:02Z

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/bea77a61e781b0e424a197064c39e76ec1cceef5/centos7/tikv-server.tar.gz

MyonKeminta · 2021-04-28T04:06:55Z

/bench

…nformation-for-lock-view

MyonKeminta · 2021-04-29T04:53:17Z

/release

sre-bot · 2021-04-29T05:17:32Z

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/f41e410a48f45d82b0f63d4674bdc8a3cbac3de0/centos7/tikv-server.tar.gz

src/storage/txn/commands/acquire_pessimistic_lock.rs

src/storage/txn/commands/mod.rs

longfangsong

Rest LGTM, but we should merge kvproto#752 and use kvproto on master first.

…nformation-for-lock-view

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

MyonKeminta · 2021-05-06T11:13:56Z

/test

…nformation-for-lock-view

MyonKeminta · 2021-05-07T08:24:46Z

/test

youjiali1995

Rest LGTM.

youjiali1995 · 2021-05-06T09:18:38Z

src/server/lock_manager/deadlock.rs

    ts: TimeStamp,
-    hashes: Vec<u64>,
+    // (hash, key)
+    // The `key` is recorded as an diagnostic information. There may be multiple keys with the same


Suggested change

// The `key` is recorded as an diagnostic information. There may be multiple keys with the same

// The `key` is recorded as diagnostic information. There may be multiple keys with the same

youjiali1995 · 2021-05-07T07:46:44Z

src/storage/txn/commands/mod.rs

    pub pr: ProcessResult,
-    // (lock, is_first_lock, wait_timeout)
-    pub lock_info: Option<(lock_manager::Lock, bool, Option<WaitTimeout>)>,
+    // (lock, key, is_first_lock, wait_timeout)


Suggested change

// (lock, key, is_first_lock, wait_timeout)

youjiali1995 · 2021-05-07T08:18:58Z

src/server/lock_manager/waiter_manager.rs

-            waiter.deadlock_with(deadlock_key_hash);
+            // The `wait_chain` doesn't include the final edge that caused the deadlock, which
+            // may reduce some deadlock RPC size. To pass it to the client, append this edge
+            // to the wait chain.


Where do you append it?

You have appended it in the deadlock detector. It should be complete. I prefer to remove these comments.

Ah sorry. This comment is about the first implementation and later I changed it.

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

ti-chi-bot · 2021-05-07T11:13:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

longfangsong
youjiali1995

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.

Details

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

youjiali1995 · 2021-05-07T11:13:47Z

/merge

ti-chi-bot · 2021-05-07T11:13:48Z

@youjiali1995: 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.

Details

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 · 2021-05-07T11:13:50Z

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

Details

Commit hash: 65d8394

ti-chi-bot · 2021-05-07T11:13:57Z

@MyonKeminta: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

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.

Details

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.

…ock is detected (tikv#10085) * code change draft Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Wrap the key and tag into a struct Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * temporarily silent the warning Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * fix build Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix incorrect assertion; add test for wait chain generating Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Move wait chain generation to a seperated function Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Add test case and some refactor Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Check the reported wait chain in integration test Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Update kvproto Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Address comments Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Address comments Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: jyz0309 <45495947@qq.com>

MyonKeminta added 6 commits April 26, 2021 14:03

code change draft

f84a98e

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

Wrap the key and tag into a struct

36ebc68

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

temporarily silent the warning

93b63c9

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

fix build

8522f3c

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

Fix incorrect assertion; add test for wait chain generating

c4cc6df

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

Move wait chain generation to a seperated function

115869c

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

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 26, 2021

MyonKeminta mentioned this pull request Apr 26, 2021

Implement Lock View (Sprint 3) pingcap/tidb#24199

Closed

20 tasks

MyonKeminta added 2 commits April 27, 2021 16:39

Add test case and some refactor

5b5912e

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

Check the reported wait chain in integration test

bea77a6

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

MyonKeminta marked this pull request as ready for review April 27, 2021 14:12

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 27, 2021

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

f41e410

…nformation-for-lock-view

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021

MyonKeminta mentioned this pull request Apr 29, 2021

executor, store: Pass the SQL digest down to pessimistic lock request pingcap/tidb#24380

Merged

2 tasks

MyonKeminta requested review from longfangsong and youjiali1995 April 30, 2021 11:38

longfangsong reviewed May 6, 2021

View reviewed changes

src/storage/txn/commands/acquire_pessimistic_lock.rs Outdated Show resolved Hide resolved

longfangsong reviewed May 6, 2021

View reviewed changes

src/storage/txn/commands/mod.rs Outdated Show resolved Hide resolved

longfangsong approved these changes May 6, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 6, 2021

youjiali1995 added the sig/transaction SIG: Transaction label May 6, 2021

MyonKeminta added 3 commits May 6, 2021 17:02

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

35076b8

…nformation-for-lock-view

Update kvproto

6b9b3ac

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

Address comments

1f19013

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

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2021

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

f08acb0

…nformation-for-lock-view

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2021

youjiali1995 reviewed May 7, 2021

View reviewed changes

Address comments

65d8394

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

youjiali1995 approved these changes May 7, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 7, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 7, 2021

Merge branch 'master' into m/record-information-for-lock-view

4b490dc

ti-chi-bot merged commit daa9354 into tikv:master May 7, 2021

MyonKeminta deleted the m/record-information-for-lock-view branch May 8, 2021 03:38

	// The `key` is recorded as an diagnostic information. There may be multiple keys with the same
	// The `key` is recorded as diagnostic information. There may be multiple keys with the same

Conversation

MyonKeminta commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

MyonKeminta commented Apr 27, 2021

Uh oh!

MyonKeminta commented Apr 27, 2021

Uh oh!

sre-bot commented Apr 27, 2021

Uh oh!

MyonKeminta commented Apr 28, 2021

Uh oh!

MyonKeminta commented Apr 29, 2021

Uh oh!

sre-bot commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

longfangsong left a comment

Choose a reason for hiding this comment

Uh oh!

MyonKeminta commented May 6, 2021

Uh oh!

MyonKeminta commented May 7, 2021

Uh oh!

youjiali1995 left a comment

Choose a reason for hiding this comment

Uh oh!

youjiali1995 May 6, 2021

Choose a reason for hiding this comment

Uh oh!

youjiali1995 May 7, 2021

Choose a reason for hiding this comment

Uh oh!

youjiali1995 May 7, 2021

Choose a reason for hiding this comment

Uh oh!

youjiali1995 May 7, 2021

Choose a reason for hiding this comment

Uh oh!

MyonKeminta May 7, 2021

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented May 7, 2021

Uh oh!

youjiali1995 commented May 7, 2021

Uh oh!

ti-chi-bot commented May 7, 2021

Uh oh!

ti-chi-bot commented May 7, 2021

Uh oh!

ti-chi-bot commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MyonKeminta commented Apr 26, 2021 •

edited

Loading