deadlock: more solid role change observer #6415

youjiali1995 · 2020-01-06T00:43:08Z

What have you changed?

To prevent potential bugs, observe more events of raftstore to change the role of the deadlock detector.

What is the type of the changes?

Improvement (a change which is an improvement to an existing feature)

How is the PR tested?

Unit test
Integration test
Manual test (add detailed scripts or steps below)
Have tested it with Schrodinger for several days.

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect `tidb-ansible`?

No

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

…-role-change-observer

MyonKeminta · 2020-01-07T06:08:41Z

src/server/lock_manager/deadlock.rs

+                    let mut leader_region_id = self.leader_region_id.lock().unwrap();
+                    if *leader_region_id == region.get_id() {
+                        *leader_region_id = INVALID_ID;
+                        self.scheduler.change_role(Role::Follower);


Is it possible that when a merge happens, a Destroy happens first and an Update follows? May there be no leader between the two message?

Yes. The Update will be coming soon since the Destroy happens.

src/server/lock_manager/deadlock.rs

AndreMouche · 2020-01-07T06:23:27Z

src/server/lock_manager/deadlock.rs

-        if is_leader_region(ctx.region()) {
-            self.change_role(role.into());
+        let region = ctx.region();
+        // A region is created first, so the leader region id must be valid.


if is_leader_region is false, please return as early as possible.

AndreMouche · 2020-01-07T06:49:59Z

src/server/lock_manager/deadlock.rs

+        let region = ctx.region();
+        // A region is created first, so the leader region id must be valid.
+        if Self::is_leader_region(region)
+            && *self.leader_region_id.lock().unwrap() == region.get_id()


Is it possible it is the leader region while the region id is not leader_region_id ? what shall we do if it happens?

If it happens, it must be a stale region and need to do nothing since region id is updated when receiving Create or Update event.

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

BusyJay · 2020-01-07T07:03:41Z

I suggest to implement deadlock manager on the TiDB side for following reasons:

it's not easy to watch leader role on TiKV side, TiKV can't guarantee in time role change notification. It's possible that there are two deadlock detectors both think they are valid leader;
TiDB already has a leader role, implement deadlock manager is just like implement another gc/ddl mechanism, it's easy and reliable;
TiKV's leader can change, region can split, replicas can be moved around for various reason at any time, it introduces unnecessary complexity and fail rate to the deadlock mechanism.

MyonKeminta · 2020-01-07T07:08:51Z

@BusyJay But TiKVs need to access the deadlock manager. I think it's not a good idea to let TiKV to access TiDB. In my opinion, we can implement it in TiKV, but adopt a similar election mechanism like TiDB's ddl/gc leader rather than relying on a special region.

BusyJay · 2020-01-07T07:11:19Z

TiDB can poll changes from TiKV just like what green GC has done. The difference is that Green GC can abort the call at the end of a round of GC, but deadlock manager will keep listening until new leader is elected.

youjiali1995 · 2020-01-07T07:36:19Z

I think an election mechanism is needed since we should make sure the leader of the deadlock detector won't change unless it is down. But it's a long-term work.

AndreMouche · 2020-01-07T08:04:37Z

Each method has advantages and disadvantages, and move the deadlock manager to tidb is a long job, maybe we can consider it in the future. For now, we need to consider if current PR can improve the efficiency to detect the leader of the deadlock_manager, and determine if it is worth to merge into v3.0.9 @youjiali1995 @BusyJay @MyonKeminta

BusyJay · 2020-01-07T08:05:10Z

make sure the leader of the deadlock detector won't change unless it is down

That's exactly how TiDB leader behaves.

BusyJay · 2020-01-07T08:07:20Z

@AndreMouche Agree. It's just an advice after seeing the effort we have been made to tweak deadlock manager.

…-role-change-observer Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

…youjiali1995/tikv into more-solid-role-change-observer

youjiali1995 · 2020-01-07T08:29:11Z

/run-all-tests

AndreMouche

LGTM

sticnarf

LGTM

MyonKeminta

LGTM

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* [3.1] external_storage: add S3 support (tikv#6209) (tikv#6536) * external_storage: add S3 support (tikv#6209) Signed-off-by: Yi Wu <yiwu@pingcap.com> Signed-off-by: kennytm <kennytm@gmail.com> * lock_manager: more metrics (tikv#6392) (tikv#6422) (tikv#6444) Signed-off-by: youjiali1995 <zlwgx1023@gmail.com> * lock_manager: update default config (tikv#6426) (tikv#6429) (tikv#6446) Signed-off-by: youjiali1995 <zlwgx1023@gmail.com> * Update rocksdb and titan. Update rust-rocksdb url (tikv#6554) Signed-off-by: Yi Wu <yiwu@pingcap.com> * raft_client: connect to other stores using peer_address (tikv#5569) (tikv#6491) * *: pick threads statistics for regions (tikv#6605) Co-authored-by: jiyingtk <1039793452@qq.com> Co-authored-by: disksing <i@disksing.com> * fix test_replica_read::test_read_after_cleanup_range_for_snap (tikv#6493) Signed-off-by: 5kbpers <tangminghua@pingcap.com> * deadlock: more solid role change observer (tikv#6415) (tikv#6431) (tikv#6445) Signed-off-by: youjiali1995 <zlwgx1023@gmail.com> * raftstore: read index message count metric (tikv#6579) (tikv#6610) Signed-off-by: 5kbpers <tangminghua@pingcap.com> * raftstore: speed up conf change (tikv#6421) (tikv#6432) Signed-off-by: Jay Lee <busyjaylee@gmail.com> * raftstore: fix a panic in read index queue (tikv#6609) (tikv#6613) Signed-off-by: qupeng <qupeng@pingcap.com> * pick tikv#6414, tikv#6487, tikv#6477 and tikv#6539 to release-3.1 (tikv#6564) * raftstore: extract batch system (tikv#6414) * apply: support yield (tikv#6487) * batch-system: add benchmark (tikv#6477) * Introduce pre transfer leader (tikv#6539) Signed-off-by: Jay Lee <BusyJayLee@gmail.com> * raftstore: set wait_merge_state to none after resuming pending state (tikv#6621) Signed-off-by: Liqi Geng <gengliqiii@gmail.com> * raftstore: learner load merge target & fix a merge network recovery bug (tikv#6623) Signed-off-by: Liqi Geng <gengliqiii@gmail.com> * change merge flow path (tikv#6617) Signed-off-by: Liqi Geng <gengliqiii@gmail.com> Co-authored-by: kennytm <kennytm@gmail.com> Co-authored-by: Lei Zhao <zlwgx1023@gmail.com> Co-authored-by: yiwu-arbug <yiwu@pingcap.com> Co-authored-by: disksing <i@disksing.com> Co-authored-by: ShuNing <nolouch@gmail.com> Co-authored-by: jiyingtk <1039793452@qq.com> Co-authored-by: pingcap-github-bot <sre-bot@pingcap.com> Co-authored-by: 5kbpers <20279863+5kbpers@users.noreply.github.com> Co-authored-by: Jay <BusyJay@users.noreply.github.com> Co-authored-by: gengliqi <gengliqiii@gmail.com>

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 added 3 commits January 1, 2020 14:29

deadlock: more solid role change notifier

ae9c970

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

check leader id in on_role_change

ed09711

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Merge branch 'master' of https://github.com/tikv/tikv into more-solid…

e41d3a7

…-role-change-observer

youjiali1995 added the component/server Component: Server label Jan 6, 2020

youjiali1995 added this to the v3.0.9 milestone Jan 6, 2020

youjiali1995 requested review from MyonKeminta and sticnarf January 6, 2020 00:43

Merge branch 'master' into more-solid-role-change-observer

38e1626

MyonKeminta reviewed Jan 7, 2020

View reviewed changes

src/server/lock_manager/deadlock.rs Outdated Show resolved Hide resolved

AndreMouche reviewed Jan 7, 2020

View reviewed changes

use CoprocessorHost::default()

68ea573

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Merge branch 'master' into more-solid-role-change-observer

3682e55

AndreMouche modified the milestones: v3.0.9, v3.1.10 Jan 7, 2020

youjiali1995 added 2 commits January 7, 2020 16:21

Merge branch 'master' of https://github.com/tikv/tikv into more-solid…

2f3d3dd

…-role-change-observer Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Merge branch 'more-solid-role-change-observer' of https://github.com/…

62a3572

…youjiali1995/tikv into more-solid-role-change-observer

Merge branch 'master' into more-solid-role-change-observer

ed4c203

AndreMouche approved these changes Jan 7, 2020

View reviewed changes

sticnarf approved these changes Jan 7, 2020

View reviewed changes

MyonKeminta approved these changes Jan 7, 2020

View reviewed changes

youjiali1995 merged commit 02b77fb into tikv:master Jan 7, 2020

youjiali1995 deleted the more-solid-role-change-observer branch January 7, 2020 09:31

youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Jan 7, 2020

deadlock: more solid role change observer (tikv#6415)

bf39991

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 mentioned this pull request Jan 7, 2020

deadlock: more solid role change observer (#6415) #6431

Merged

sre-bot pushed a commit that referenced this pull request Jan 7, 2020

deadlock: more solid role change observer (#6415) (#6431)

da69418

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Jan 9, 2020

deadlock: more solid role change observer (tikv#6415) (tikv#6431)

c0a424e

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 mentioned this pull request Jan 9, 2020

deadlock: more solid role change observer (#6415) (#6431) #6445

Merged

AndreMouche removed this from the v3.1.10 milestone Jan 10, 2020

youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Feb 10, 2020

deadlock: more solid role change observer (tikv#6415) (tikv#6431)

aed7dfc

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 added a commit that referenced this pull request Feb 14, 2020

deadlock: more solid role change observer (#6415) (#6431) (#6445)

d334f90

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

c1ay pushed a commit to c1ay/tikv that referenced this pull request May 9, 2020

deadlock: more solid role change observer (tikv#6415)

e7da72f

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deadlock: more solid role change observer #6415

deadlock: more solid role change observer #6415

youjiali1995 commented Jan 6, 2020 •

edited

MyonKeminta Jan 7, 2020

youjiali1995 Jan 7, 2020

AndreMouche Jan 7, 2020

youjiali1995 Jan 7, 2020

AndreMouche Jan 7, 2020

youjiali1995 Jan 7, 2020

BusyJay commented Jan 7, 2020

MyonKeminta commented Jan 7, 2020

BusyJay commented Jan 7, 2020

youjiali1995 commented Jan 7, 2020

AndreMouche commented Jan 7, 2020

BusyJay commented Jan 7, 2020

BusyJay commented Jan 7, 2020

youjiali1995 commented Jan 7, 2020

AndreMouche left a comment

sticnarf left a comment

MyonKeminta left a comment

deadlock: more solid role change observer #6415

deadlock: more solid role change observer #6415

Conversation

youjiali1995 commented Jan 6, 2020 • edited

What have you changed?

What is the type of the changes?

How is the PR tested?

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Does this PR affect tidb-ansible?

MyonKeminta Jan 7, 2020

Choose a reason for hiding this comment

youjiali1995 Jan 7, 2020

Choose a reason for hiding this comment

AndreMouche Jan 7, 2020

Choose a reason for hiding this comment

youjiali1995 Jan 7, 2020

Choose a reason for hiding this comment

AndreMouche Jan 7, 2020

Choose a reason for hiding this comment

youjiali1995 Jan 7, 2020

Choose a reason for hiding this comment

BusyJay commented Jan 7, 2020

MyonKeminta commented Jan 7, 2020

BusyJay commented Jan 7, 2020

youjiali1995 commented Jan 7, 2020

AndreMouche commented Jan 7, 2020

BusyJay commented Jan 7, 2020

BusyJay commented Jan 7, 2020

youjiali1995 commented Jan 7, 2020

AndreMouche left a comment

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

MyonKeminta left a comment

Choose a reason for hiding this comment

youjiali1995 commented Jan 6, 2020 •

edited

Does this PR affect `tidb-ansible`?