store/tikv: Make RangeTaskRunner support dividing task by multiple regions #10482

MyonKeminta · 2019-05-15T07:27:14Z

What problem does this PR solve?

This PR continue improves #10379 and should be merged after that PR.
Also requires pingcap/kvproto#396 and tikv/pd#1535 .

The implementation of RangeTaskRunner in #10379 divides the range by regions in one thread, and dispatch these tasks to the worker threads. But if the concurrency is set too high or the tasks costs too short time to finish, then the single thread which is responsible to fetch region meta and divide the range will be the bottleneck of RangeTaskRunner. This PR makes it support divides the ranges by multiple regions to reduce the main thread or RangeTaskRunner.

What is changed and how it works?

A new interface ScanRegions of PD is added in pdpb: add ScanRegions kvproto#396 .
Add method BatchLoadRegions to RegionCache. It invokes ScanRegions of PDClient, fill the scanned regions to RegionCache, and return the end key of the last scanned region.
In RangeTaskRunner, the main threads invokes BatchLoadRegions to divide the region into multiple tasks, and the number of regions of each divided subtask is configurable.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Side effects

Possible performance regression
Increased code complexity

Related changes

None

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

…concurrent-resolve-lock

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

…yonKeminta/tidb into misono/concurrent-resolve-lock

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

…multi-region

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

codecov · 2019-05-15T07:39:51Z

Codecov Report

Merging #10482 into master will decrease coverage by 0.0204%.
The diff coverage is 69.0721%.

@@               Coverage Diff                @@
##             master     #10482        +/-   ##
================================================
- Coverage   81.0157%   80.9952%   -0.0205%     
================================================
  Files           419        419                
  Lines         88747      88825        +78     
================================================
+ Hits          71899      71944        +45     
- Misses        11629      11652        +23     
- Partials       5219       5229        +10

…concurrent-resolve-lock

…multi-region

MyonKeminta · 2019-05-15T08:07:28Z

/run-all-tests pd=pr/1535

zhangjinpeng87 · 2019-05-21T12:40:43Z

store/tikv/region_cache.go

+			}
+			regions = append(regions, region)
+		}
+		if len(regions) == 0 {


Duplicate condition checking with L531.

It's not the same, since not every item in metas will be added to regions

zhangjinpeng87 · 2019-05-21T12:41:36Z

store/tikv/region_cache.go

+		if len(regions) == 0 {
+			return nil, errors.New("receive Regions with no peer")
+		}
+		if len(regions) < len(metas) {


Seems len(regions) always equal to len(metas)?

Regions with no leader will be ignored.

…range-task-multi-region

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

store/tikv/region_cache.go

disksing · 2019-06-10T07:30:12Z

the rest LGTM

…range-task-multi-region

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

MyonKeminta · 2019-06-18T06:01:52Z

/run-all-tests

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

…yonKeminta/tidb into misono/range-task-multi-region

disksing · 2019-06-19T08:16:49Z

LGTM

MyonKeminta · 2019-06-19T08:20:30Z

@disksing Do you think this should be cherry-picked to release-3.0?

disksing · 2019-06-19T08:47:13Z

I think yes. PTAL @zhangjinpeng1987

…range-task-multi-region

MyonKeminta · 2019-06-19T11:21:48Z

@lysu Could you please help me check if I've used the new refactored region cache the correct way?

lonng · 2019-06-19T12:24:04Z

store/mockstore/mocktikv/cluster.go

+	defer c.RUnlock()
+
+	regions := make([]*Region, 0, len(c.regions))
+	for _, region := range c.regions {


You can use copy(regions, c.regions) directly.

c.regions is a map and I only needs its values. It seems that copy doesn't work well here.

lonng · 2019-06-19T12:27:07Z

store/mockstore/mocktikv/cluster.go

+	c.RLock()
+	defer c.RUnlock()
+
+	regions := make([]*Region, 0, len(c.regions))


We only need limit regions but copy the entire slice here. It's OK because the mocktikv will not contain a lot of regions.

The regions is in arbitrary order and ScanRegions needs to scan regions by key. I get all regions here and do a sorting below. I think this will be faster than finding regions by key for at most limit times, because the mock cluster doesn't support quickly finding region by key. You can see that in GetRegionByKey method, it does a linear searching on c.regions.

lonng · 2019-06-19T12:29:18Z

store/mockstore/mocktikv/cluster.go

+	for _, region := range regions {
+		leader := region.leaderPeer()
+		if leader == nil {
+			leader = &metapb.Peer{}


Seems violate the semantics of this function if we return invalid leader.

This is the same as PD's ScanRegions API. It uses a repeated field for leaders so that it can't be nil. Therefore we judge if a region has no leader by checking if the leader's ID is 0.

…range-task-multi-region

MyonKeminta · 2019-06-20T07:17:53Z

/run-all-tests
Please note that there are a lot of changes in go.mod and go.sum. Maybe they are caused by upgrading PD, but I'm not sure. @disksing PTAL.

MyonKeminta · 2019-06-20T07:28:42Z

/run-unit-test

zhangjinpeng87

LGTM

MyonKeminta and others added 24 commits May 7, 2019 11:57

Finish range task runner

d938516

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

Replace resolveLock's implementation

9176d2c

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

Set backoff

65f0cc7

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

Address comments; add test for task splitting

85ace8e

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

Remove unused constant

6600a62

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

Address comments; remove unnecessary ctx check

53fec9b

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

Adjust code sequence; Add more assertions to tests

53b37b9

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

Move subrange handling logic to handler

b347983

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

Add metrics

7260a84

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

Address comments

cc9aa5e

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

Merge branch 'master' into misono/concurrent-resolve-lock

a747bbb

Fix build

881751e

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

Add metrics for range task push duration

55e8d6e

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

Address comments

58d74da

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

Address comments

d208c2f

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

Merge branch 'master' of https://github.com/pingcap/tidb into misono/…

cae457f

…concurrent-resolve-lock

Merge branch 'master' into misono/concurrent-resolve-lock

9b3e52b

Support both auto and manual concurrency

1d400b2

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

Merge branch 'misono/concurrent-resolve-lock' of https://github.com/M…

f125ed6

…yonKeminta/tidb into misono/concurrent-resolve-lock

* Update dependency PD

4428231

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

Implement ScanRegions; Add BatchLoadRegions to region cache

3b27b58

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

Fix import order

913072e

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

Merge branch 'misono/concurrent-resolve-lock' into misono/range-task-…

f05e885

…multi-region

RangeTaskRunner: Support dividing regions by multiple regions

5cc2139

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

MyonKeminta added component/tikv status/DNM labels May 15, 2019

MyonKeminta added 2 commits May 15, 2019 15:56

Merge branch 'master' of https://github.com/pingcap/tidb into misono/…

2196a63

…concurrent-resolve-lock

Merge branch 'misono/concurrent-resolve-lock' into misono/range-task-…

ccc27cd

…multi-region

zhangjinpeng87 reviewed May 21, 2019

View reviewed changes

MyonKeminta added 2 commits June 4, 2019 12:27

Merge branch 'master' of https://github.com/pingcap/tidb into misono/…

5604dc3

…range-task-multi-region

Fix scanRegions

0c3d6cd

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

disksing reviewed Jun 10, 2019

View reviewed changes

store/tikv/region_cache.go Outdated Show resolved Hide resolved

MyonKeminta and others added 4 commits June 18, 2019 13:09

Merge branch 'master' of https://github.com/pingcap/tidb into misono/…

8b31e04

…range-task-multi-region

Set 1 region per task in TestRangeTaskError

72c3ab8

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

Address comments

cb6d635

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

Merge branch 'master' into misono/range-task-multi-region

fb0880f

MyonKeminta added 2 commits June 18, 2019 14:23

Address comments

9053d47

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

Merge branch 'misono/range-task-multi-region' of https://github.com/M…

b15cbaa

…yonKeminta/tidb into misono/range-task-multi-region

disksing added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 19, 2019

Merge branch 'master' of https://github.com/pingcap/tidb into misono/…

fd3fffc

…range-task-multi-region

lonng reviewed Jun 19, 2019

View reviewed changes

Merge branch 'master' of https://github.com/pingcap/tidb into misono/…

e9c9cbd

…range-task-multi-region

MyonKeminta and others added 2 commits June 20, 2019 16:17

Merge branch 'master' into misono/range-task-multi-region

f33bfba

Merge branch 'master' into misono/range-task-multi-region

ba5ceb0

zhangjinpeng87 approved these changes Jun 20, 2019

View reviewed changes

MyonKeminta merged commit c12abec into pingcap:master Jun 20, 2019

MyonKeminta deleted the misono/range-task-multi-region branch June 20, 2019 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store/tikv: Make RangeTaskRunner support dividing task by multiple regions #10482

store/tikv: Make RangeTaskRunner support dividing task by multiple regions #10482

MyonKeminta commented May 15, 2019

codecov bot commented May 15, 2019 •

edited

Loading

MyonKeminta commented May 15, 2019

zhangjinpeng87 May 21, 2019

MyonKeminta Jun 18, 2019

zhangjinpeng87 May 21, 2019

MyonKeminta Jun 18, 2019

disksing commented Jun 10, 2019

MyonKeminta commented Jun 18, 2019

disksing commented Jun 19, 2019

MyonKeminta commented Jun 19, 2019

disksing commented Jun 19, 2019

MyonKeminta commented Jun 19, 2019

lonng Jun 19, 2019 •

edited

Loading

MyonKeminta Jun 20, 2019 •

edited

Loading

lonng Jun 19, 2019

MyonKeminta Jun 20, 2019 •

edited

Loading

lonng Jun 19, 2019

MyonKeminta Jun 20, 2019

MyonKeminta commented Jun 20, 2019

MyonKeminta commented Jun 20, 2019

zhangjinpeng87 left a comment

store/tikv: Make RangeTaskRunner support dividing task by multiple regions #10482

store/tikv: Make RangeTaskRunner support dividing task by multiple regions #10482

Conversation

MyonKeminta commented May 15, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented May 15, 2019 • edited Loading

Codecov Report

MyonKeminta commented May 15, 2019

zhangjinpeng87 May 21, 2019

Choose a reason for hiding this comment

MyonKeminta Jun 18, 2019

Choose a reason for hiding this comment

zhangjinpeng87 May 21, 2019

Choose a reason for hiding this comment

MyonKeminta Jun 18, 2019

Choose a reason for hiding this comment

disksing commented Jun 10, 2019

MyonKeminta commented Jun 18, 2019

disksing commented Jun 19, 2019

MyonKeminta commented Jun 19, 2019

disksing commented Jun 19, 2019

MyonKeminta commented Jun 19, 2019

lonng Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

MyonKeminta Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

lonng Jun 19, 2019

Choose a reason for hiding this comment

MyonKeminta Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

lonng Jun 19, 2019

Choose a reason for hiding this comment

MyonKeminta Jun 20, 2019

Choose a reason for hiding this comment

MyonKeminta commented Jun 20, 2019

MyonKeminta commented Jun 20, 2019

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

codecov bot commented May 15, 2019 •

edited

Loading

lonng Jun 19, 2019 •

edited

Loading

MyonKeminta Jun 20, 2019 •

edited

Loading

MyonKeminta Jun 20, 2019 •

edited

Loading