raftstore: rely on the all-target-peer-exist guarantee during merging by gengliqi · Pull Request #7672 · tikv/tikv

gengliqi · 2020-04-27T17:40:06Z

Signed-off-by: Liqi Geng gengliqiii@gmail.com

What problem does this PR solve?

Problem Summary:

In the past, we assume some target peers may not exist during merging. It introduces many complexities that we need to think about how to handle it.

Also, there are some cases we can not handle.

For example, a target peer does not exist during merging. Its corresponding source peer is waiting for it to be created. Then other target peers do the conf change and remove it(though it never existed). Next, the target region merges to another target region.

In this case, the waiting source peer can not find out whether it should be removed. (It's very difficult, the PD knows nothing because the target region has merged to another one)

Luckily, in tikv/pd#839 (the earliest PR about merging in PD), it checks the target peer's pending peer count must equal to zero.
So we can rely on the all-target-peer-exist guarantee to make it easier and save our brain power.

What is changed and how it works?

What's Changed:

Rely on the all-target-peer-exist guarantee during merging.
Remove some code about handling some cases when a target peer may not exist during merging.

Related changes

Need to cherry-pick to the release branch

Check List

Tests

No code

Side effects

We should check it twice whether the earliest version can give us this guarantee.
Otherwise, it will break backward compatibility.

Release note

Fix a case that a peer can not be removed when its store is isolated during multiple merge process.

BusyJay · 2020-04-27T18:06:21Z

I suggest to keep those code for safe check. Otherwise, incorrect implement of PD can lead to disaster.

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

components/raftstore/src/store/fsm/peer.rs

sre-bot · 2020-05-09T03:16:47Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi · 2020-05-09T10:08:11Z

/run-all-tests

BusyJay · 2020-05-11T12:47:57Z

components/raftstore/src/store/fsm/peer.rs

I think this is useless. It's optimized out in release mode, and we don't test debug build with PD, so the check can't really detect anything about real PD, it just checks for nothing.

Maybe using something else instead.
I think we need a method to assert in release mode in the development environment.
There are some cases that using assert! directly is a little bit overkill in the production environment but necessary in the development environment. Then if enough tests have been passed, maybe we can change it to assert!.

I don't think it needs to be assert in any cases. Even this is not hold, it doesn't effect normal operations of TiKV, it just leads to some unremovable replicas. I think just an alert is enough and leave an API to allow user to clean up stale replicas.

It's ok to not change to assert. but an assert method is needed to detect some bugs in our tests in the development environment.

Yes, that's why I suggest an alert.

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

BusyJay · 2020-05-21T11:13:26Z

components/raftstore/src/store/config.rs

+    #[doc(hidden)]
+    #[serde(skip_serializing)]
+    #[config(skip)]
+    pub ensure_all_target_peer_exist: bool,


Should be a better name so that it can be reused in other cases besides merge.

BusyJay · 2020-05-21T11:13:38Z

components/raftstore/src/store/config.rs

            future_poll_size: 1,
            hibernate_regions: true,
            early_apply: true,
+            ensure_all_target_peer_exist: true,


Default should be false.

It should be true. If so, an error log will be printed when something is wrong and breaks this guarantee.

How? From what I can see, it just debug_assert.

I misrepresented it. It should be false only in test just for passing some tests and not panic. debug_assert will be replaced to another one.

components/raftstore/src/store/fsm/peer.rs

tests/integrations/raftstore/test_merge.rs

components/test_raftstore/src/pd.rs

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

BusyJay · 2020-05-27T11:56:50Z

components/raftstore/src/store/peer.rs

                    }
+                } else {
+                    if ctx.cfg.dev_assert {
+                        assert!(false, "{} failed to get peer {} from cache", self.tag, id);


How about just panic?

BusyJay · 2020-05-27T12:04:50Z

components/test_raftstore/src/pd.rs

    region_replication_status: HashMap<u64, RegionReplicationStatus>,
+
+    // for merging
+    pub merge_ensure_all_target_peer_exist: bool,


How about check_merge_target_integrity?

BusyJay · 2020-05-27T12:15:28Z

components/raftstore/src/store/fsm/peer.rs

        }
+        drop(meta);

-        // Check whether target peer is set to tombstone already.


Can they be kept in case PD is not working as expected?

I don't think so. The query to PD can only handle some cases not all of them. I think it has very little possibility that PD does not ensure all target peer exists because I have already checked them. Also, if PD really does not ensure it, the consequence is not serious and we can remove the region manually.

BusyJay · 2020-05-27T12:25:58Z

components/raftstore/src/store/fsm/peer.rs

+    /// It should be called when target region is not in region map in memory.
+    /// If everything is ok, the answer should always be true because PD should ensure all target peers exist.
+    /// So if not, error log will be printed and return false.
+    fn is_local_merge_target_region_fresher(&self, target_region: &metapb::Region) -> Result<bool> {


How about is_merge_target_outdated?

I think the original name is fine 🤣 because I want to emphasize local_target_region

The original name is too long, we usually use three to four words for naming. And using fresher here is very weird. A more up to date local target region means target_region to be checked is outdated.

BusyJay · 2020-05-27T12:41:27Z

components/raftstore/src/store/fsm/peer.rs

+                target_region.get_region_epoch(),
+                target_state.get_region().get_region_epoch(),
+            ) {
+                return Ok(true);


What if the source peer applies slowly and is still in an outdated merge?

I don't get it. This function is used for the source peer to check whether its target peer is fresher. If so, the merge process may need to rollback or the merge succeeds and the source peer needs to destroy itself.

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

BusyJay

fresher in comments need to be updated too. Rest LGTM.

BusyJay · 2020-05-29T12:23:44Z

components/raftstore/src/store/fsm/peer.rs

-                    "target_region_id" => region_id,
-                );
-                return Ok(false);
+            Ok(true) => Err(box_err!("region {} is destroyed", target_region_id)),


This is not correct. Target peer doesn't have to be destroyed to reach this branch.

If the target peer is not destroyed, it should in region map, but it's not.

BusyJay · 2020-05-29T13:12:49Z

@Connor1996 @NingLin-P PTAL

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

Connor1996

LGTM

sre-bot · 2020-06-01T08:13:22Z

/run-all-tests

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot · 2020-06-03T04:11:59Z

cherry pick to release-3.1 in PR #8003

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot · 2020-06-03T04:13:04Z

cherry pick to release-4.0 in PR #8004

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot · 2020-06-03T04:13:45Z

cherry pick to release-3.0 in PR #8005

…#7672) (#8005) Co-authored-by: Liqi Geng <gengliqiii@gmail.com> Co-authored-by: qupeng <qupeng@pingcap.com>

gengliqi requested review from BusyJay and Connor1996 April 27, 2020 17:43

rely on PD to ensure all target peer exist

ba54942

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi force-pushed the remove-some-code branch from b9486bc to ba54942 Compare May 6, 2020 08:12

BusyJay reviewed May 6, 2020

View reviewed changes

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

fix failed tests

01cd206

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi force-pushed the remove-some-code branch from 6a99f1c to 01cd206 Compare May 9, 2020 03:22

gengliqi added 2 commits May 9, 2020 14:55

add test

2d54ef8

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

merge master

0c91989

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi requested a review from BusyJay May 9, 2020 07:00

make clippy happy

9c605d4

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi force-pushed the remove-some-code branch from 455a4d3 to 9c605d4 Compare May 9, 2020 09:49

gengliqi added sig/raft Component: Raft, RaftStore, etc. sig/raft type/bugfix This PR fixes a bug. and removed sig/raft labels May 11, 2020

BusyJay reviewed May 11, 2020

View reviewed changes

merge master

3576c7f

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi force-pushed the remove-some-code branch from caba904 to 3576c7f Compare May 19, 2020 05:12

BusyJay reviewed May 21, 2020

View reviewed changes

gengliqi requested a review from NingLin-P May 22, 2020 06:35

Connor1996 reviewed May 22, 2020

View reviewed changes

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

Connor1996 reviewed May 22, 2020

View reviewed changes

add dev_assert

fa5b921

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

gengliqi force-pushed the remove-some-code branch 3 times, most recently from 8f14b07 to e6dcdd9 Compare May 22, 2020 11:13

gengliqi force-pushed the remove-some-code branch from 4ff0a7c to f8cfc9c Compare May 27, 2020 11:43

BusyJay reviewed May 27, 2020

View reviewed changes

gengliqi added 3 commits May 28, 2020 15:50

address comments

a2b657a

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

address comments

b0b4c26

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

add log

27202e4

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

BusyJay reviewed May 29, 2020

View reviewed changes

address comments

03f8769

Signed-off-by: Liqi Geng <gengliqiii@gmail.com>

Connor1996 approved these changes Jun 1, 2020

View reviewed changes

BusyJay approved these changes Jun 1, 2020

View reviewed changes

gengliqi added the status/can-merge Indicates a PR has been approved by a committer. label Jun 1, 2020

Merge branch 'master' into remove-some-code

4918aea

sre-bot merged commit ed337a4 into tikv:master Jun 1, 2020

gengliqi added needs-cherry-pick-release-3.0 Type: Need cherry pick to release 3.0 needs-cherry-pick-release-3.1 Type: Need cherry pick to release 3.1 needs-cherry-pick-release-4.0 Type: Need cherry pick to release 4.0 labels Jun 3, 2020

sre-bot pushed a commit to sre-bot/tikv that referenced this pull request Jun 3, 2020

cherry pick tikv#7672 to release-3.1

a707db2

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot mentioned this pull request Jun 3, 2020

raftstore: rely on the all-target-peer-exist guarantee during merging (#7672) #8003

Closed

sre-bot pushed a commit to sre-bot/tikv that referenced this pull request Jun 3, 2020

cherry pick tikv#7672 to release-4.0

3cedceb

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot mentioned this pull request Jun 3, 2020

raftstore: rely on the all-target-peer-exist guarantee during merging (#7672) #8004

Closed

sre-bot pushed a commit to sre-bot/tikv that referenced this pull request Jun 3, 2020

cherry pick tikv#7672 to release-3.0

d1c29d3

Signed-off-by: sre-bot <sre-bot@pingcap.com>

sre-bot mentioned this pull request Jun 3, 2020

raftstore: rely on the all-target-peer-exist guarantee during merging (#7672) #8005

Merged

gengliqi deleted the remove-some-code branch June 5, 2020 07:26

gengliqi mentioned this pull request Jun 5, 2020

raftstore: rely on the all-target-peer-exist guarantee during merging(#7672) #8048

Merged

hicqu added a commit that referenced this pull request Jun 28, 2020

raftstore: rely on the all-target-peer-exist guarantee during merging (…

74c8a65

…#7672) (#8005) Co-authored-by: Liqi Geng <gengliqiii@gmail.com> Co-authored-by: qupeng <qupeng@pingcap.com>

hbisheng mentioned this pull request Dec 26, 2024

raftstore: fix missing pending peers in region heartbeat after split #18032

Merged

9 tasks

Conversation

gengliqi commented Apr 27, 2020 • 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!

BusyJay commented Apr 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sre-bot commented May 9, 2020

Uh oh!

gengliqi commented May 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliqi May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay commented May 29, 2020

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Jun 1, 2020

gengliqi commented Apr 27, 2020 •

edited

Loading

gengliqi May 29, 2020 •

edited

Loading