api:add api for history_hot_region info;cluster add storage for hot region;config add config for hot-region storage #3989

qidi1 · 2021-08-15T08:19:31Z

What problem does this PR solve?

Proposal: pingcap/tidb#27487
Associated PR:tikv/tidb#27224
Associated Issue: pingcap/tidb#25281
pingcap/tidb#27373
pingcap/tidb#27374

What is changed and how it works?

Store hot-region information regularly,Provide api interface for requesting historical hot-region information

Check List

Tests

Unit test

Code changes

Has configuration change
add hot-regions-reserved-days and hot-regions-write-interval in scheduler,hot-regions-reserved-days means how many days of information are kept,hot-regions-write-interval means the time interval for pulling information
Has HTTP API interfaces change (Don't forget to add the declarative for API)
Add post api /hotspot/regions/history
Has persistent data change
store hot region info

Side effects

Possible performance regression
store hot region data may have some performance to pd,under 1000 regions,10 min write intervals context,to read all data in a month needs 18s, 1.7s is needed for every month Leveldb compaction

Related changes

Associated PR: pingcap/tidb#27224

Release note

Store hot-region information regularly,Provide  api interface for requesting historical hot-region information

ti-chi-bot · 2021-08-15T08:19:33Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: qidi1 <1083369179@qq.com>

codecov · 2021-08-16T02:36:35Z

Codecov Report

Merging #3989 (f803c06) into master (1db2398) will increase coverage by 0.11%.
The diff coverage is 65.97%.

@@            Coverage Diff             @@
##           master    #3989      +/-   ##
==========================================
+ Coverage   74.65%   74.77%   +0.11%     
==========================================
  Files         258      258              
  Lines       26337    26413      +76     
==========================================
+ Hits        19662    19749      +87     
+ Misses       4929     4902      -27     
- Partials     1746     1762      +16

Flag	Coverage Δ
unittests	`74.77% <65.97%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tools/pd-ctl/pdctl/command/hot_command.go	`59.34% <56.94%> (-3.40%)`	⬇️
server/core/hot_region_storage.go	`74.68% <77.77%> (+0.34%)`	⬆️
server/api/router.go	`97.25% <100.00%> (+0.01%)`	⬆️
server/handler.go	`48.15% <100.00%> (+9.36%)`	⬆️
server/tso/local_allocator.go	`71.62% <0.00%> (-6.76%)`	⬇️
server/election/lease.go	`85.71% <0.00%> (-6.35%)`	⬇️
server/region_syncer/client.go	`77.46% <0.00%> (-4.23%)`	⬇️
pkg/dashboard/adapter/manager.go	`79.78% <0.00%> (-3.20%)`	⬇️
client/base_client.go	`81.52% <0.00%> (-2.72%)`	⬇️
server/member/member.go	`65.24% <0.00%> (-1.61%)`	⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1db2398...f803c06. Read the comment docs.

conf/config.toml

conf/simconfig.toml

server/cluster/hot_region_storage.go

server/cluster/hot_region_storage_test.go

ti-chi-bot · 2021-08-16T02:47:29Z

@IcePigZDB: Request changes is only allowed for the reviewers in list.

In response to this:

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.

conf/config.toml

disksing · 2021-08-17T07:13:20Z

There are enough reviewers so I'm unassigning myself :)

nolouch · 2021-08-17T06:47:08Z

conf/simconfig.toml

@@ -29,3 +29,6 @@ leader-schedule-limit = 32
 region-schedule-limit = 128
 replica-schedule-limit = 32
 merge-schedule-limit = 32
+## TODO:Find a better place to put this config
+hot-regions-reserved-days= "30"


In simconfig, use 0 to close it is ok, it is the simulator environment.

nolouch · 2021-08-17T06:48:00Z

server/api/router.go

@@ -140,6 +140,7 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
 	apiRouter.HandleFunc("/hotspot/regions/write", hotStatusHandler.GetHotWriteRegions).Methods("GET")
 	apiRouter.HandleFunc("/hotspot/regions/read", hotStatusHandler.GetHotReadRegions).Methods("GET")
 	apiRouter.HandleFunc("/hotspot/stores", hotStatusHandler.GetHotStores).Methods("GET")
+	apiRouter.HandleFunc("/hotspot/regions/history", hotStatusHandler.GetHistoryHotRegions).Methods("POST")


use "Get" more suitable?

nolouch · 2021-08-17T11:00:17Z

server/handler.go

+	storeSet := make(map[uint64]bool)
+	peerSet := make(map[uint64]bool)
+	for _, id := range request.RegionIDs {
+		regionSet[id] = true


Why need request region IDs and peerIDs? I think only StoresIds and KeyRange are reasonable.

nolouch · 2021-08-17T11:01:07Z

server/statistics/history_hot_region.go

@@ -0,0 +1,50 @@
+package statistics


need to add license like other files.

nolouch · 2021-08-17T11:01:47Z

server/cluster/hot_region_storage.go

@@ -0,0 +1,320 @@
+package cluster


nolouch · 2021-08-17T11:06:24Z

BTW, I think your PR can split into three PRs:

storage
config
API

more small PRs review more quickly. And the quality is more guaranteed.

nolouch · 2021-08-19T05:20:59Z

BTW, you can also supports pd-ctl command to check hot history region.

Yisaer

I recommended submitting single pr for storage part first

Yisaer · 2021-08-19T05:41:37Z

server/cluster/hot_region_storage.go

+		for {
+			select {
+			case <-ticker.C:
+				if h.member.IsLeader() {


I recommended that HotRegionStorage should be controlled by whether the member is elected as leader during leader campaign to guarantee only the leader could flush data instead of checking leader identity in its own logic.

Yisaer · 2021-08-19T05:42:45Z

server/cluster/hot_region_storage.go

+	if err != nil {
+		return nil, err
+	}
+	hotRegionInfoCtx, hotRegionInfoCancle := context.WithCancel(ctx)


Suggested change

hotRegionInfoCtx, hotRegionInfoCancle := context.WithCancel(ctx)

hotRegionInfoCtx, hotRegionInfoCancel := context.WithCancel(ctx)

Yisaer · 2021-08-19T05:53:06Z

server/cluster/hot_region_storage.go

+	// make delete happened in defaultDeleteTime clock.
+	now := time.Now()
+	next := now.Add(time.Hour * 24)
+	next = time.Date(next.Year(), next.Month(), next.Day(), defaultDeleteTime, 0, 0, 0, next.Location())


The logic here is kind of weird. I think this could be simplified as running background job 24 hours once here, and simply delete the data which timestamp prefix ranges belong to [0, now - 7 days ago).

Here, I want the delete function run at a time point, such as one clock every day.

Yisaer · 2021-08-19T05:53:18Z

server/cluster/hot_region_storage.go

+	now := time.Now()
+	next := now.Add(time.Hour * 24)
+	next = time.Date(next.Year(), next.Month(), next.Day(), defaultDeleteTime, 0, 0, 0, next.Location())
+	t := time.NewTicker(next.Sub(now))


defer ticker.stop as soon as possible after creating ticker.

Yisaer · 2021-08-19T05:54:14Z

server/cluster/hot_region_storage.go

+	ticker := time.NewTicker(h.pullInterval)
+	go func() {
+		defer ticker.Stop()


Yisaer · 2021-08-19T05:54:51Z

server/cluster/hot_region_storage.go

+			select {
+			case <-ticker.C:
+				if h.member.IsLeader() {
+					if err := h.pullHotRegionInfo(); err != nil {


If error happened, we should continue this loop.

Yisaer · 2021-08-19T05:57:57Z

server/cluster/hot_region_storage.go

+type HotRegionStorage struct {
+	*kv.LeveldbKV
+	encryptionKeyManager *encryptionkm.KeyManager
+	mu                   sync.RWMutex


I suggest to use mutex like following in order to make it clear that which variables are under mutex controlled.

mu struct { sync.Mutex .... controlled variable here }

Yisaer · 2021-08-19T06:01:34Z

server/cluster/hot_region_storage.go

+	if err := h.LeveldbKV.Write(batch, nil); err != nil {
+		return errs.ErrLevelDBWrite.Wrap(err).GenWithStackByCause()
+	}
+	h.batchHotInfo = make(map[string]*statistics.HistoryHotRegion)


What's this line used for?

…ve AsLeader Signed-off-by: qidi1 <1083369179@qq.com>

Signed-off-by: qidi1 <1083369179@qq.com>

ti-chi-bot · 2021-09-27T16:46:38Z

@qidi1: PR needs rebase.

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 kubernetes/test-infra repository.

ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 15, 2021

ti-chi-bot requested review from disksing and rleungx August 15, 2021 08:19

qidi1 mentioned this pull request Aug 15, 2021

server.api add api for history_hot_region info;server.cluster add storage for hot region;sever.config add 'hot-regions-reserved-days' and 'hot-region-write-interval' #3988

Closed

qidi1 force-pushed the qidi1 branch from ba4f78d to b0aa65f Compare August 15, 2021 09:44

qidi1 added 5 commits August 15, 2021 20:24

config:add config for hot region history

8f45266

Signed-off-by: qidi1 <1083369179@qq.com>

api:add history-hot-region interface

5daff0f

Signed-off-by: qidi1 <1083369179@qq.com>

statistics:add comminicate struct

0f744af

Signed-off-by: qidi1 <1083369179@qq.com>

cluster:add hot region storage

d95e987

Signed-off-by: qidi1 <1083369179@qq.com>

server:add method to get hot region info

c77c560

Signed-off-by: qidi1 <1083369179@qq.com>

qidi1 force-pushed the qidi1 branch from aebba2b to c77c560 Compare August 15, 2021 12:29

qidi1 added 2 commits August 15, 2021 20:30

conf: add config in config toml

f03c4e5

Signed-off-by: qidi1 <1083369179@qq.com>

fix style error in code

bdf7ca5

Signed-off-by: qidi1 <1083369179@qq.com>

IcePigZDB mentioned this pull request Aug 16, 2021

infoschema,planner,execuor: support hot history region pingcap/tidb#27224

Closed

12 tasks

Merge branch 'master' into qidi1

b0b6e0f

IcePigZDB suggested changes Aug 16, 2021

View reviewed changes

qidi1 force-pushed the qidi1 branch 3 times, most recently from ea2a150 to 4731301 Compare August 16, 2021 07:22

nolouch self-requested a review August 17, 2021 02:57

nolouch reviewed Aug 17, 2021

View reviewed changes

conf/config.toml Outdated Show resolved Hide resolved

disksing removed their request for review August 17, 2021 07:13

nolouch reviewed Aug 17, 2021

View reviewed changes

Yisaer reviewed Aug 19, 2021

View reviewed changes

qidi1 added 3 commits September 12, 2021 16:52

cluster:fix bug every region see as read,AsPeer include AsLeader,remo…

b4be83e

…ve AsLeader Signed-off-by: qidi1 <1083369179@qq.com>

command:add test for hot history regions

f9b6797

Signed-off-by: qidi1 <1083369179@qq.com>

cluster:add test for hot region storage

cfab956

Signed-off-by: qidi1 <1083369179@qq.com>

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2021

qidi1 added 2 commits September 14, 2021 16:41

api:change return nil to err,change reponse of 500 code

bc0f0c2

Signed-off-by: qidi1 <1083369179@qq.com>

cluster:add test in tests/server/core

7b9233e

Signed-off-by: qidi1 <1083369179@qq.com>

qidi1 force-pushed the qidi1 branch from 9ee964e to 7b9233e Compare September 14, 2021 12:08

qidi1 mentioned this pull request Sep 14, 2021

core:solve bug in hot_region_storage #4121

Merged

14 tasks

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2021

qidi1 force-pushed the qidi1 branch from 2a5c9b7 to 7b9233e Compare September 14, 2021 12:56

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2021

qidi1 added 2 commits September 14, 2021 21:02

cluster:remove some unuse code

f3d6135

Signed-off-by: qidi1 <1083369179@qq.com>

api:remove unuse code

e931026

Signed-off-by: qidi1 <1083369179@qq.com>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2021

qidi1 added 3 commits September 15, 2021 19:37

api:remove append in leaderSet and learnerSet

a47777c

Signed-off-by: qidi1 <1083369179@qq.com>

ctl:append if is_leader and is_learner not sepcfic

31d5264

Signed-off-by: qidi1 <1083369179@qq.com>

ctl:change test name to TestHistoryHotRegions

50ebc1b

Signed-off-by: qidi1 <1083369179@qq.com>

qidi1 force-pushed the qidi1 branch from 360a292 to 50ebc1b Compare September 16, 2021 04:27

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2021

qidi1 force-pushed the qidi1 branch 2 times, most recently from f73e4a4 to 64e8802 Compare September 18, 2021 01:48

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2021

core:change core.HotRegionType from string to enum

566d0eb

Signed-off-by: qidi1 <1083369179@qq.com>

qidi1 force-pushed the qidi1 branch from c80c3e0 to 566d0eb Compare September 18, 2021 02:04

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2021

Merge branch 'master' into qidi1

f803c06

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2021

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2021

qidi1 closed this Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api:add api for history_hot_region info;cluster add storage for hot region;config add config for hot-region storage #3989

api:add api for history_hot_region info;cluster add storage for hot region;config add config for hot-region storage #3989

qidi1 commented Aug 15, 2021 •

edited

Loading

ti-chi-bot commented Aug 15, 2021

codecov bot commented Aug 16, 2021 •

edited

Loading

ti-chi-bot commented Aug 16, 2021

disksing commented Aug 17, 2021

nolouch Aug 17, 2021

nolouch Aug 17, 2021

nolouch Aug 17, 2021

nolouch Aug 17, 2021

nolouch Aug 17, 2021

nolouch commented Aug 17, 2021

nolouch commented Aug 19, 2021

Yisaer left a comment

Yisaer Aug 19, 2021

Yisaer Aug 19, 2021

Yisaer Aug 19, 2021

qidi1 Aug 23, 2021

Yisaer Aug 19, 2021

Yisaer Aug 19, 2021

Yisaer Aug 19, 2021

Yisaer Aug 19, 2021

Yisaer Aug 19, 2021

ti-chi-bot commented Sep 27, 2021

	hotRegionInfoCtx, hotRegionInfoCancle := context.WithCancel(ctx)
	hotRegionInfoCtx, hotRegionInfoCancel := context.WithCancel(ctx)

api:add api for history_hot_region info;cluster add storage for hot region;config add config for hot-region storage #3989

api:add api for history_hot_region info;cluster add storage for hot region;config add config for hot-region storage #3989

Conversation

qidi1 commented Aug 15, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Side effects

Related changes

Release note

ti-chi-bot commented Aug 15, 2021

codecov bot commented Aug 16, 2021 • edited Loading

Codecov Report

ti-chi-bot commented Aug 16, 2021

disksing commented Aug 17, 2021

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

nolouch commented Aug 17, 2021

nolouch commented Aug 19, 2021

Yisaer left a 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-chi-bot commented Sep 27, 2021

qidi1 commented Aug 15, 2021 •

edited

Loading

codecov bot commented Aug 16, 2021 •

edited

Loading