executor: add retriver for tikv_region_peers by IcePigZDB · Pull Request #30657 · pingcap/tidb

IcePigZDB · 2021-12-13T03:06:47Z

What problem does this PR solve?

Issue Number: close #31050
Problem Summary:
The optimization idea is to add TikvRegionPeersExtractor to push down STORE_ID and REGION_ID and add tikvRegionPeersRetriever to fetch regions information from PD.

What is changed and how it works?

ref issue: Push down optimisation of TIKV_REGION_STATUS and TiKV_REGION_PEERS #28330

should merged after relative planner PR planner: add extractor for tikv_region_peers #30656

Time cost comparison:

# select count(*) from information_schema.tikv_region_peers;
+----------+
| count(*) |
+----------+
|   700476 |
+----------+
new: 1 row in set (10.30 sec)
old: 1 row in set (10.29 sec)
# almost cost the same time when fetch all regionsInfo

# select * from tikv_region_peers where region_id=395005;
+-----------+---------+----------+------------+-----------+--------+--------------+
| REGION_ID | PEER_ID | STORE_ID | IS_LEARNER | IS_LEADER | STATUS | DOWN_SECONDS |
+-----------+---------+----------+------------+-----------+--------+--------------+
|    395005 |  410054 |        4 |          0 |         1 | NORMAL |         NULL |
+-----------+---------+----------+------------+-----------+--------+--------------+
new: 1 row in set (0.00 sec)
old: 1 row in set (11.37 sec)
# faster when specify one region_id

# select * from tikv_region_peers where region_id in (395005,395015,395017);
+-----------+---------+----------+------------+-----------+--------+--------------+
| REGION_ID | PEER_ID | STORE_ID | IS_LEARNER | IS_LEADER | STATUS | DOWN_SECONDS |
+-----------+---------+----------+------------+-----------+--------+--------------+
|    395005 |  410054 |        4 |          0 |         1 | NORMAL |         NULL |
|    395015 |  410061 |        4 |          0 |         1 | NORMAL |         NULL |
|    395017 |  759007 |        1 |          0 |         1 | NORMAL |         NULL |
+-----------+---------+----------+------------+-----------+--------+--------------+
new: 3 rows in set (0.00 sec)
old: 3 rows in set (10.10 sec)
# faster when specify three region_id

# select count(*) from tikv_region_peers where store_id=4;
+----------+
| count(*) |
+----------+
|   182409 |
+----------+
new: 1 row in set (5.45 sec)
old: 1 row in set (10.38 sec)
# faster when specify one store_id

# select count(*) from tikv_region_peers where region_id=395005 and store_id=4;
+----------+
| count(*) |
+----------+
|        1 |
+----------+
new: 1 row in set (4.80 sec)
old: 1 row in set (11.03 sec)
# faster when specify one store_id

# select * from tikv_region_peers where region_id in (395005,395015,395017) and store_id=4;
+-----------+---------+----------+------------+-----------+--------+--------------+
| REGION_ID | PEER_ID | STORE_ID | IS_LEARNER | IS_LEADER | STATUS | DOWN_SECONDS |
+-----------+---------+----------+------------+-----------+--------+--------------+
|    395005 |  410054 |        4 |          0 |         1 | NORMAL |         NULL |
|    395015 |  410061 |        4 |          0 |         1 | NORMAL |         NULL |
+-----------+---------+----------+------------+-----------+--------+--------------+
new: 2 rows in set (4.88 sec)
old: 2 rows in set (10.69 sec)
# faster when specify one store_id

# select * from tikv_region_peers where region_id in (395005,395015,395017) and store_id=0;
Empty set (0.01 sec)
Empty set (10.83 sec)
# faster when specify one store_id

# select count(*) from tikv_region_peers where store_id in (4,7);
+----------+
| count(*) |
+----------+
|   364012 |
+----------+
1 row in set (9.32 sec)
1 row in set (10.30 sec)
# closer when specify two store_id

# select * from tikv_region_peers where region_id in (395005,395015,395017) and store_id in (1,4);
+-----------+---------+----------+------------+-----------+--------+--------------+
| REGION_ID | PEER_ID | STORE_ID | IS_LEARNER | IS_LEADER | STATUS | DOWN_SECONDS |
+-----------+---------+----------+------------+-----------+--------+--------------+
|    395005 |  410054 |        4 |          0 |         1 | NORMAL |         NULL |
|    395015 |  410061 |        4 |          0 |         1 | NORMAL |         NULL |
|    395017 |  759007 |        1 |          0 |         1 | NORMAL |         NULL |
+-----------+---------+----------+------------+-----------+--------+--------------+
new: 3 rows in set (12.90 sec)
old: 3 rows in set (12.03 sec)
# slower when specify two or more store_id

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ref: pingcap#28330 Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

ti-chi-bot · 2021-12-13T03:06:48Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

sre-bot · 2021-12-13T03:07:26Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

zhouqiang-cl · 2021-12-13T07:53:48Z

/rebuild

executor/memtable_reader.go

JmPotato · 2021-12-20T10:10:31Z

The rest LGTM basically.

Co-authored-by: JmPotato <ghzpotato@gmail.com>

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

IcePigZDB · 2021-12-22T00:48:31Z

/run-all-tests

sre-bot · 2021-12-22T01:02:20Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/334e618c39f0b1701174adccbe537030dd1d96ed

executor/memtable_reader.go

JmPotato · 2021-12-22T03:13:05Z

executor/memtable_reader.go

+		if len(e.extractor.RegionIDs) < 1 {
+			return e.packTiKVRegionPeersRows(regionsInfoByStoreID)
+		}


(1/3) I think this judgment is unnecessary since it will not enter the len(e.extractor.RegionIDs) > 0 logic below either.

Refactor and move them before intersect.
The predicates extract by extractor is and case only, and len(regionsInfoByStoreID) == 0 may include two cases:

no store_id predicate: Do not intersect

no corresponding store regionsInfo: Intersect

so we should check the length of extracted predicate rather than result slice regionsInfoByStoreID or regionsInfoByRegionID.

// len(regionsInfoByStoreID) == 0 include two cases: // 1. no store_id predicate 2. no corresponding store regionsInfo if len(e.extractor.RegionIDs) == 0 { return e.packTiKVRegionPeersRows(regionsInfoByStoreID) } if len(e.extractor.StoreIDs) == 0 { return e.packTiKVRegionPeersRows(regionsInfoByRegionID) } // intersect for _, region := range regionsInfoByRegionID { if _, ok := regionMap[region.ID]; ok { regionsInfo = append(regionsInfo, region) } }

Oh, I missed that part. Nice catch 👍. But what about the or condition 🤔?

Oh, I missed that part. Nice catch 👍. But what about the or condition 🤔?

if len(e.extractor.RegionIDs) < 1 { return e.packTiKVRegionPeersRows(regionsInfoByStoreID) }

In origin version implemention,the use of regionsInfoByStoreID and regionsInfoByRegionID make or difficult to use.

How about the newest verion of implmention. I think we need to exclude unwanted regions got from store_id by region_id when specify both store_id and region_id, so I keep the regionsInfoByStoreID.

JmPotato · 2021-12-22T03:13:22Z

executor/memtable_reader.go

+		if len(e.extractor.StoreIDs) < 1 {
+			return e.packTiKVRegionPeersRows(regionsInfoByRegionID)
+		}


(2/3) Ditto.

JmPotato · 2021-12-22T03:14:56Z

executor/memtable_reader.go

+	// intersect
+	for _, region := range regionsInfoByRegionID {
+		if _, ok := regionMap[region.ID]; ok {
+			regionsInfo = append(regionsInfo, region)
+		}
+	}
+	return e.packTiKVRegionPeersRows(regionsInfo)


(3/3) We can intersect both regionsInfoByStoreID and regionsInfoByRegionID here, which would make the code more easy and clear.

Co-authored-by: JmPotato <ghzpotato@gmail.com>

rleungx · 2021-12-22T03:21:25Z

util/pdapi/const.go

+	HotHistory   = "/pd/api/v1/hotspot/regions/history"
+	Regions      = "/pd/api/v1/regions"
+	RegionByID   = "/pd/api/v1/region/id/"
+	StoreRegions = "/pd/api/v1/regions/store/"


Prefer to keep the same format, e.g. /pd/api/v1/regions/store

Done, and hence add + "/" in implemention code

pdapi.StoreRegions+"/"+strconv.FormatUint(storeID, 10), nil, &regionsInfo) pdapi.RegionByID+"/"+strconv.FormatUint(regionID, 10), nil, &regionInfo)

rleungx · 2021-12-22T03:24:04Z

executor/memtable_reader.go

+	if e.extractor.SkipRequest || e.retrieved {
+		return nil, nil
+	}
+	e.retrieved = true


We should change it to true when it is actually retrieved

The position of clusterServerInfoRetriever is at the begining too. More specifically, tikvRegionsPeersRetriver's implementation has many return cases, it would be redundant if put e.retrieved = true before returns.

rleungx · 2021-12-22T03:26:45Z

executor/memtable_reader.go

+
+	var regionsInfo, regionsInfoByStoreID, regionsInfoByRegionID []helper.RegionInfo
+	regionMap := make(map[int64]struct{})
+	if len(e.extractor.StoreIDs) > 0 {


not necessary

Refactor and remove if len(e.extractor.StoreIDs) > 0 {.

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

nolouch · 2022-01-25T04:00:12Z

ptal @JmPotato @rleungx

JmPotato · 2022-01-25T09:48:58Z

/merge

ti-chi-bot · 2022-01-25T09:49:03Z

@JmPotato: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

Details

In response to this:

/merge

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.

crazycs520 · 2022-02-07T03:54:05Z

/merge

ti-chi-bot · 2022-02-07T03:54:08Z

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

Details

Commit hash: d81c254

crazycs520 · 2022-02-07T04:11:13Z

/merge

crazycs520 · 2022-02-07T07:40:48Z

/merge

crazycs520 · 2022-02-07T08:15:34Z

/merge

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2022-02-07T08:48:43Z

cherry pick to release-5.4 in PR #32132

IcePigZDB added 4 commits December 13, 2021 03:00

store/helper: add GetStoreRegionsInfo by store id

0c83460

ref: pingcap#28330 Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

executor: remove setDataForTikVRegionPeers

67336e4

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

executor: add retriver for tikv_region_peers

4cadd79

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

executor: add retriver test for tikv_region_peers

4922cfe

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 13, 2021

IcePigZDB changed the title ~~Optikvregionpeers executor~~ executor: add retriver for tikv_region_peers Dec 13, 2021

IcePigZDB mentioned this pull request Dec 13, 2021

Push down optimisation of TIKV_REGION_STATUS and TiKV_REGION_PEERS #28330

Open

5 tasks

IcePigZDB marked this pull request as ready for review December 13, 2021 03:09

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 Dec 13, 2021

IcePigZDB mentioned this pull request Dec 13, 2021

Inforschema, executor: add push down for tikv_region_peers #28211

Closed

12 tasks

IcePigZDB mentioned this pull request Dec 15, 2021

planner: add extractor for tikv_region_peers #30656

Merged

12 tasks

JmPotato reviewed Dec 20, 2021

View reviewed changes

executor/memtable_reader.go Outdated Show resolved Hide resolved

IcePigZDB and others added 6 commits December 20, 2021 18:17

Update executor/memtable_reader.go

cc3c7a4

Co-authored-by: JmPotato <ghzpotato@gmail.com>

Merge branch 'master' into optikvregionpeers-executor

d7ef520

executor: use map[int64]struct{} in tikvRegionPeersRetriever

d8057dc

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

executor: fix typo in intersect case

b88f2c3

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

executor: remove debug code in retriver test

f0770da

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

Merge branch 'master' into optikvregionpeers-executor

47a4841

JmPotato reviewed Dec 22, 2021

View reviewed changes

Update executor/memtable_reader.go

50accf5

Co-authored-by: JmPotato <ghzpotato@gmail.com>

rleungx reviewed Dec 22, 2021

View reviewed changes

executor, helper, util: keep pdapi frefix format consistent

8ce64f1

Signed-off-by: IcePigZDB <icepigzdb@gmail.com>

rleungx approved these changes Jan 25, 2022

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 Jan 25, 2022

Merge branch 'master' into optikvregionpeers-executor

fdb5339

ti-chi-bot added the do-not-merge/needs-linked-issue label Jan 27, 2022

IcePigZDB and others added 2 commits January 27, 2022 14:26

Merge branch 'master' into optikvregionpeers-executor

79390f5

Merge branch 'master' into optikvregionpeers-executor

d81c254

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

nolouch removed the do-not-merge/needs-linked-issue label Feb 7, 2022

Merge branch 'master' into optikvregionpeers-executor

b2db512

ti-chi-bot added do-not-merge/needs-linked-issue needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. and removed do-not-merge/needs-linked-issue labels Feb 7, 2022

nolouch removed the needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. label Feb 7, 2022

Merge branch 'master' into optikvregionpeers-executor

538c308

ti-chi-bot added the needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. label Feb 7, 2022

Merge branch 'master' into optikvregionpeers-executor

334e618

ti-chi-bot merged commit 43b818b into pingcap:master Feb 7, 2022

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 7, 2022

cherry pick pingcap#30657 to release-5.4

31e8657

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Feb 7, 2022

executor: add retriver for tikv_region_peers (#30657) #32132

Closed

13 tasks

Conversation

IcePigZDB commented Dec 13, 2021 • edited by nolouch 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?

Time cost comparison:

Check List

Release note

Uh oh!

ti-chi-bot commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented Dec 13, 2021

Uh oh!

zhouqiang-cl commented Dec 13, 2021

Uh oh!

Uh oh!

JmPotato commented Dec 20, 2021

Uh oh!

IcePigZDB commented Dec 22, 2021

Uh oh!

sre-bot commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IcePigZDB Dec 22, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolouch commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JmPotato commented Jan 25, 2022

Uh oh!

ti-chi-bot commented Jan 25, 2022

Uh oh!

crazycs520 commented Feb 7, 2022

Uh oh!

ti-chi-bot commented Feb 7, 2022

Uh oh!

crazycs520 commented Feb 7, 2022

Uh oh!

crazycs520 commented Feb 7, 2022

Uh oh!

crazycs520 commented Feb 7, 2022

Uh oh!

ti-srebot commented Feb 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

IcePigZDB commented Dec 13, 2021 •

edited by nolouch

Loading

ti-chi-bot commented Dec 13, 2021 •

edited

Loading

sre-bot commented Dec 22, 2021 •

edited

Loading

IcePigZDB Dec 22, 2021 •

edited

Loading

nolouch commented Jan 25, 2022 •

edited

Loading