raftstore: move get_region_approximate_size to split check worker #9081

sleepymole · 2020-11-20T08:48:48Z

What problem does this PR solve?

When peer's approximate size or keys is none，region heartbeat need to directly read rocksdb to get size and keys. It will affect other region's heartbeat. This PR move get_region_approximate_size and get_region_approximate_keys to split worker's thread so that most regions' heartbeat will go smoothly.

What is changed and how it works?

What's Changed:

Add a new task type GetRegionApproximateSizeAndKeys to split checker. For this task, split check worker will read rocksdb to get size and keys, and execute an callback.

Check List

Tests

Unit test
Integration test

Release note

No release note.

components/raftstore/src/store/worker/split_check.rs

components/raftstore/src/store/worker/pd.rs

Signed-off-by: gozssky <[email protected]>

Little-Wallace

LGTM

ti-srebot · 2020-11-26T08:38:53Z

@Little-Wallace, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIGs: raft(slack),scheduling(slack).

components/raftstore/src/store/worker/split_check.rs

Signed-off-by: gozssky <[email protected]>

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

Signed-off-by: gozssky <[email protected]>

gengliqi · 2020-12-02T17:15:47Z

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

+        // Roughly estimate the size and keys and then let split checker to update it.
+        self.fsm.peer.approximate_size =
+            self.fsm.peer.approximate_size.map(|x| x / new_region_count);
+        self.fsm.peer.approximate_keys =
+            self.fsm.peer.approximate_keys.map(|x| x / new_region_count);
+        if let Err(e) = self.ctx.split_check_scheduler.schedule(
+            SplitCheckTask::GetRegionApproximateSizeAndKeys {
+                region: self.fsm.peer.region().clone(),
+                pending_tasks: Arc::new(AtomicU64::new(1)),
+                cb: Box::new(move |_, _| {}),
+            },
+        ) {
+            error!(
+                "failed to schedule split check task";
+                "region_id" => self.fsm.region_id(),
+                "peer_id" => self.fsm.peer_id(),
+                "err" => ?e,
+            );
+        }


If it's not leader, this task is very wasteful and the approximate_size & approximate_key are no need to be updated.

And I think self.fsm.peer.heartbeat_pd should be called here if it's a leader.

If it's not leader, this task is very wasteful and the approximate_size & approximate_key are no need to be updated.

Oh, I didn't notice here in the last update. I will add some check later.

And I think self.fsm.peer.heartbeat_pd should be called here if it's a leader.

heartbeat_pd is called a few lines before here.

@gengliqi I have updated. PTAL.

I think we can move these codes to the upper if is_leader.

I have updated again. Please have a look.

Signed-off-by: gozssky <[email protected]>

gengliqi · 2020-12-04T06:24:09Z

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

+        // It's not correct anymore, so set it to None to let split checker update it.
+        self.fsm.peer.approximate_size = None;


Hmmm, I think we should remove these lines?

This is for non-leader peers. As the previous comment says, the size is not correct anymore.

If we use the estimated size for non-leader peers but not to update it size immediately, I don't know whether there are
potential risks?

If self.fsm.peer.approximate_size is none, the latter estimated_size is always none.

There is no potential risk because the approximate_size and approximate_keys are meaningful only on the leader.

Should it be meaningful if a follower becomes a leader?

If self.fsm.peer.approximate_size is none, the latter estimated_size is always none.

Sorry, I make a mistake here.

gengliqi · 2020-12-04T07:23:15Z

components/raftstore/src/store/worker/split_check.rs

+                let _ = self.router.send(
+                    region.get_id(),
+                    CasualMessage::RegionApproximateSize { size },
+                );
+                let _ = self.router.send(
+                    region.get_id(),
+                    CasualMessage::RegionApproximateKeys { keys },
+                );


These data may be sent to a different peer instead of the expected one in rare circumstances. I find it is the same as the original code of RegionApproximateSize/Keys, which also has this problem. It seems that the side effect is little but it's risky. I will file an issue for that.

Sounds like a potential risk. Please let me know what caused it to happen when you are free.

Signed-off-by: gozssky <[email protected]>

ti-srebot · 2020-12-04T08:10:54Z

@gengliqi, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: scheduling(slack).

gengliqi · 2020-12-04T08:12:04Z

/lgtm

gengliqi · 2020-12-04T08:12:44Z

/merge

ti-srebot · 2020-12-04T08:13:14Z

/run-all-tests

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2020-12-04T08:30:23Z

cherry pick to release-4.0 in PR #9185

) (#9185) Signed-off-by: ti-srebot <[email protected]> Signed-off-by: gozssky <[email protected]>

sleepymole changed the title ~~move get_region_approximate_size to split check worker~~ raftstore: move get_region_approximate_size to split check worker Nov 20, 2020

Little-Wallace added sig/raft Component: Raft, RaftStore, etc. component/pd-client Component: pd status/WIP labels Nov 20, 2020

Little-Wallace reviewed Nov 21, 2020

View reviewed changes

components/raftstore/src/store/worker/split_check.rs Outdated Show resolved Hide resolved

Little-Wallace reviewed Nov 23, 2020

View reviewed changes

components/raftstore/src/store/worker/pd.rs Outdated Show resolved Hide resolved

Little-Wallace reviewed Nov 23, 2020

View reviewed changes

components/raftstore/src/store/worker/pd.rs Outdated Show resolved Hide resolved

sleepymole and others added 12 commits November 23, 2020 13:05

send size and keys to router

45def28

Signed-off-by: gozssky <[email protected]>

rename

730257f

Signed-off-by: gozssky <[email protected]>

add pending heartbeat pd

9c398f8

Signed-off-by: gozssky <[email protected]>

add test_region_heartbeat_when_size_or_keys_is_none

def6e6c

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

deac59c

use pending_pd_heartbeat_tasks

46a8e6d

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

f7fffc0

rerange code

c549ebd

Signed-off-by: gozssky <[email protected]>

typo

fe1edde

Signed-off-by: gozssky <[email protected]>

avoid too many heartbeats

3414886

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

294b4b4

Merge branch 'master' into fix-pd-hb-block

20b00e9

Little-Wallace approved these changes Nov 26, 2020

View reviewed changes

Little-Wallace removed the status/WIP label Nov 26, 2020

Connor1996 reviewed Nov 26, 2020

View reviewed changes

components/raftstore/src/store/worker/split_check.rs Show resolved Hide resolved

sleepymole and others added 2 commits November 27, 2020 17:07

estimate size and keys after split

70f33c0

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

98b4eeb

sleepymole requested a review from Little-Wallace November 27, 2020 09:09

use and_then instead of map_or

23acc0f

Signed-off-by: gozssky <[email protected]>

Little-Wallace reviewed Nov 27, 2020

View reviewed changes

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

sleepymole and others added 2 commits November 27, 2020 18:45

bug fix

9b18961

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

298b30b

only leader need to update size and keys

f5780de

Signed-off-by: gozssky <[email protected]>

gengliqi reviewed Dec 2, 2020

View reviewed changes

sleepymole and others added 5 commits December 3, 2020 14:37

only leader need to update size and keys

d5a7233

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

c2eee6f

estimate size before heartbeat_pd

02aad56

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

bf9aa3d

remove extra newline

55d47e0

Signed-off-by: gozssky <[email protected]>

gengliqi reviewed Dec 4, 2020

View reviewed changes

sleepymole and others added 2 commits December 4, 2020 15:48

fix estimated size and keys

7d87f61

Signed-off-by: gozssky <[email protected]>

Merge branch 'master' into fix-pd-hb-block

7a6e4a9

gengliqi approved these changes Dec 4, 2020

View reviewed changes

gengliqi added the sig/raft Component: Raft, RaftStore, etc. label Dec 4, 2020

ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 4, 2020

ti-srebot approved these changes Dec 4, 2020

View reviewed changes

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 4, 2020

ti-srebot merged commit e8e008d into tikv:master Dec 4, 2020

ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Dec 4, 2020

cherry pick tikv#9081 to release-4.0

b823810

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Dec 4, 2020

raftstore: move get_region_approximate_size to split check worker (#9081) #9185

Merged

sleepymole deleted the fix-pd-hb-block branch December 4, 2020 08:30

glorv mentioned this pull request Dec 7, 2020

Ingest failed due to EpochNotMatch error pingcap/tidb-lightning#436

Open

Little-Wallace mentioned this pull request Dec 8, 2020

PD worker is blocked by get_region_approximate_size #9024

Closed

ti-srebot added a commit that referenced this pull request Dec 14, 2020

raftstore: move get_region_approximate_size to split check worker (#9081

8243fdc

) (#9185) Signed-off-by: ti-srebot <[email protected]> Signed-off-by: gozssky <[email protected]>

gengliqi mentioned this pull request Apr 30, 2021

Region may not trigger split check no matter how large it is #10111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore: move get_region_approximate_size to split check worker #9081

raftstore: move get_region_approximate_size to split check worker #9081

sleepymole commented Nov 20, 2020 •

edited

Loading

Little-Wallace left a comment

ti-srebot commented Nov 26, 2020

gengliqi Dec 2, 2020

gengliqi Dec 2, 2020

sleepymole Dec 2, 2020

sleepymole Dec 3, 2020

gengliqi Dec 3, 2020

sleepymole Dec 4, 2020

gengliqi Dec 4, 2020

sleepymole Dec 4, 2020

sleepymole Dec 4, 2020

gengliqi Dec 4, 2020

gengliqi Dec 4, 2020

sleepymole Dec 4, 2020

gengliqi Dec 4, 2020

sleepymole Dec 4, 2020

ti-srebot commented Dec 4, 2020

gengliqi commented Dec 4, 2020

gengliqi commented Dec 4, 2020

ti-srebot commented Dec 4, 2020

ti-srebot commented Dec 4, 2020

		// It's not correct anymore, so set it to None to let split checker update it.
		self.fsm.peer.approximate_size = None;

raftstore: move get_region_approximate_size to split check worker #9081

raftstore: move get_region_approximate_size to split check worker #9081

Conversation

sleepymole commented Nov 20, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Little-Wallace left a comment

Choose a reason for hiding this comment

ti-srebot commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-srebot commented Dec 4, 2020

gengliqi commented Dec 4, 2020

gengliqi commented Dec 4, 2020

ti-srebot commented Dec 4, 2020

ti-srebot commented Dec 4, 2020

sleepymole commented Nov 20, 2020 •

edited

Loading