server/cluster: add regionState to record abnormal regions #5272

LLThomas · 2022-07-06T03:26:57Z

What problem does this PR solve?

Issue Number: Ref #4684

What is changed and how does it work?

In order to track abnormal regions, this pr has built a generic checker that currently only detects regions that have expired due to machine downtime.

server/cluster: add a regionState to record abnormal regions

Check List

Tests

Unit test

Release note

None.

Signed-off-by: LLThomas <zs033@qq.com>

ti-chi-bot · 2022-07-06T03:26:58Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato

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: LLThomas <zs033@qq.com>

codecov · 2022-07-06T05:40:20Z

Codecov Report

Merging #5272 (b840ec2) into master (c0a623f) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head b840ec2 differs from pull request most recent head ad34dfc. Consider uploading reports for the commit ad34dfc to get more accurate results

@@            Coverage Diff             @@
##           master    #5272      +/-   ##
==========================================
+ Coverage   75.67%   75.74%   +0.06%     
==========================================
  Files         312      313       +1     
  Lines       30966    30991      +25     
==========================================
+ Hits        23435    23473      +38     
+ Misses       5520     5515       -5     
+ Partials     2011     2003       -8

Flag	Coverage Δ
unittests	`75.74% <100.00%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
server/cluster/coordinator.go	`71.16% <100.00%> (-1.33%)`	⬇️
server/cluster/metrics.go	`100.00% <100.00%> (ø)`
server/cluster/region_stat.go	`100.00% <100.00%> (ø)`
pkg/errs/errs.go	`75.00% <0.00%> (-25.00%)`	⬇️
server/storage/storage.go	`67.30% <0.00%> (-3.85%)`	⬇️
server/region_syncer/server.go	`82.96% <0.00%> (-3.30%)`	⬇️
server/region_syncer/client.go	`85.07% <0.00%> (-2.99%)`	⬇️
server/storage/endpoint/meta.go	`61.36% <0.00%> (-2.28%)`	⬇️
server/schedule/hbstream/heartbeat_streams.go	`72.72% <0.00%> (-2.03%)`	⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

JmPotato

Why not implement this feature through RegionStatistics? Checker should be more like a struct that will generate certain operators for scheduling purpose.

LLThomas · 2022-07-06T06:37:27Z

Why not implement this feature through RegionStatistics? Checker should be more like a struct that will generate certain operators for scheduling purpose.

You are right, I misunderstood the issue. I'll make a change.

Signed-off-by: LLThomas <zs033@qq.com>

rleungx · 2022-07-06T09:00:29Z

FYI, #3901 is another way to implement it. But there is an extra cost since we need to scan more times.

HunDunDM · 2022-07-11T08:10:18Z

server/cluster/region_stat.go

+
+// region state type
+const (
+	RegionStateDown regionStateType = 1 << iota


Variables of type private should not be public.
Why use 1 << iota?

HunDunDM · 2022-07-11T08:11:11Z

server/cluster/region_stat.go

+type regionState struct {
+	sync.RWMutex
+	opt    *config.PersistOptions
+	states map[regionStateType]map[uint64]*core.RegionInfo


Replace map with a fixed-length array.

HunDunDM · 2022-07-11T08:11:49Z

server/cluster/region_stat.go

+
+// regionState is used to record abnormal regions.
+type regionState struct {
+	sync.RWMutex


The current design does not require a lock?

HunDunDM · 2022-07-11T08:14:01Z

server/cluster/region_stat.go

+}
+
+// GetRegionStateByType gets the states of the region by types. The regions here need to be cloned, otherwise, it may cause data race problems.
+func (r *regionState) getRegionStateByType(typ regionStateType) []*core.RegionInfo {


Is this function only used in tests?

HunDunDM · 2022-07-11T08:16:33Z

server/cluster/region_stat.go

+}
+
+// Check verifies a region's state, recording it if need.
+func (r *regionState) observe(region *core.RegionInfo) {


Maybe we could pass in a slice of regions and compute time.Now only once.

HunDunDM · 2022-07-11T08:17:15Z

server/cluster/region_stat.go

+}
+
+// Collect collects the metrics of the regions' states.
+func (r *regionState) collect() {


How to clean up a region if it does not exist in the region tree?

Signed-off-by: LLThomas <zs033@qq.com>

JmPotato · 2022-07-13T13:08:16Z

server/cluster/region_stat.go

+)
+
+// clearThreadhold indicates when we do cleanup operations.
+// When the number of times the collect() function is executed is over than this threshold,


Suggested change

// When the number of times the collect() function is executed is over than this threshold,

// When the number of times the collect() function is executed exceeds this threshold,

JmPotato · 2022-07-13T13:11:06Z

server/cluster/region_stat.go

+	regionStateTypeLen
+)
+
+// regionState is used to record abnormal regions.


I think we should add some comments to show its difference with the RegionStatistics.

Suggested change

// regionState is used to record abnormal regions.

// regionState is used to record abnormal regions which could not be detected by the region heartbeat.

// For example, the down regions won’t send any heartbeat anymore.

JmPotato · 2022-07-13T13:13:43Z

server/cluster/region_stat.go

+// Collect collects the metrics of the regions' states.
+func (r *regionState) collect() {


What about renaming this function to collectAndClean to indicate the possible clean-up work?

Signed-off-by: LLThomas <zs033@qq.com>

nolouch · 2022-07-25T05:48:57Z

PTAL @HunDunDM

LLThomas · 2022-08-01T12:31:08Z

Picture of down-region-count :

HunDunDM · 2022-08-03T03:48:45Z

server/cluster/region_stat.go

+
+// region state type
+const (
+	regionStateDown regionStateType = iota


Consider replacing Down with NoLeader to prevent confusion with down-peer?

HunDunDM · 2022-08-03T03:51:50Z

server/cluster/metrics.go

@@ -119,6 +119,14 @@ var (
 			Name:      "store_sync",
 			Help:      "The state of store sync config",
 		}, []string{"address", "state"})
+
+	regionStateGauge = prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{


A bit of a duplication with regionStatusGauge of server/statistics/metrics.go.

HunDunDM · 2022-08-03T03:52:54Z

server/cluster/region_stat.go

+type regionState struct {
+	cluster schedule.Cluster
+	states  [regionStateTypeLen]map[uint64]*core.RegionInfo
+	count   int


Use a more specific name.

nolouch · 2022-08-17T06:00:18Z

ptal @LLThomas

Signed-off-by: LLThomas <zs033@qq.com>

ti-chi-bot · 2022-10-10T09:21:30Z

@LLThomas: 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.

LLThomas added 2 commits July 6, 2022 10:59

record down regions

1e45a89

Signed-off-by: LLThomas <zs033@qq.com>

Merge branch 'tikv:master' into disconnect-region

0a5d6c8

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 6, 2022

ti-chi-bot requested review from disksing and rleungx July 6, 2022 03:27

LLThomas added 2 commits July 6, 2022 11:33

make check

a4d97f1

Signed-off-by: LLThomas <zs033@qq.com>

Merge branch 'master' into disconnect-region

4cc1adc

JmPotato reviewed Jul 6, 2022

View reviewed changes

LLThomas added 2 commits July 6, 2022 16:25

make some changes

5ed5f9d

Signed-off-by: LLThomas <zs033@qq.com>

fix ci error

cc04ce2

Signed-off-by: LLThomas <zs033@qq.com>

LLThomas requested a review from JmPotato July 6, 2022 08:53

LLThomas changed the title ~~checker: add a checker to check abnormal regions~~ server/cluster: add a regionState to record abnormal regions Jul 6, 2022

LLThomas changed the title ~~server/cluster: add a regionState to record abnormal regions~~ server/cluster: add regionState to record abnormal regions Jul 6, 2022

HunDunDM reviewed Jul 11, 2022

View reviewed changes

fix code

643c1f3

Signed-off-by: LLThomas <zs033@qq.com>

JmPotato reviewed Jul 13, 2022

View reviewed changes

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

LLThomas added 2 commits July 17, 2022 02:38

fix comments

9b224cd

Signed-off-by: LLThomas <zs033@qq.com>

Merge branch 'master' into disconnect-region

c58be4a

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

Merge branch 'master' into disconnect-region

3af57d8

LLThomas marked this pull request as ready for review July 18, 2022 14:16

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 Jul 18, 2022

LLThomas added 2 commits July 20, 2022 13:33

Merge branch 'tikv:master' into disconnect-region

731a17c

test collectAndClean()

b840ec2

Signed-off-by: LLThomas <zs033@qq.com>

disksing removed their request for review July 25, 2022 04:29

JmPotato approved these changes Aug 1, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 1, 2022

LLThomas requested a review from HunDunDM August 2, 2022 04:38

HunDunDM reviewed Aug 3, 2022

View reviewed changes

LLThomas added 5 commits August 18, 2022 13:08

change position

6666710

Signed-off-by: LLThomas <zs033@qq.com>

fix ci/cd error

51b4040

Signed-off-by: LLThomas <zs033@qq.com>

fix fmt error

21658f6

Signed-off-by: LLThomas <zs033@qq.com>

for test

830426b

Signed-off-by: LLThomas <zs033@qq.com>

rollback

ad34dfc

Signed-off-by: LLThomas <zs033@qq.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server/cluster: add regionState to record abnormal regions #5272

server/cluster: add regionState to record abnormal regions #5272

LLThomas commented Jul 6, 2022 •

edited

Loading

ti-chi-bot commented Jul 6, 2022 •

edited

Loading

codecov bot commented Jul 6, 2022 •

edited

Loading

JmPotato left a comment

LLThomas commented Jul 6, 2022

rleungx commented Jul 6, 2022

HunDunDM Jul 11, 2022

HunDunDM Jul 11, 2022

HunDunDM Jul 11, 2022

HunDunDM Jul 11, 2022

HunDunDM Jul 11, 2022

HunDunDM Jul 11, 2022

JmPotato Jul 13, 2022

JmPotato Jul 13, 2022

JmPotato Jul 13, 2022

nolouch commented Jul 25, 2022

LLThomas commented Aug 1, 2022

HunDunDM Aug 3, 2022

HunDunDM Aug 3, 2022

HunDunDM Aug 3, 2022

nolouch commented Aug 17, 2022

ti-chi-bot commented Oct 10, 2022

	// When the number of times the collect() function is executed is over than this threshold,
	// When the number of times the collect() function is executed exceeds this threshold,

	// regionState is used to record abnormal regions.
	// regionState is used to record abnormal regions which could not be detected by the region heartbeat.
	// For example, the down regions won’t send any heartbeat anymore.

		// Collect collects the metrics of the regions' states.
		func (r *regionState) collect() {

server/cluster: add regionState to record abnormal regions #5272

Are you sure you want to change the base?

server/cluster: add regionState to record abnormal regions #5272

Conversation

LLThomas commented Jul 6, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Jul 6, 2022 • edited Loading

codecov bot commented Jul 6, 2022 • edited Loading

Codecov Report

JmPotato left a comment

Choose a reason for hiding this comment

LLThomas commented Jul 6, 2022

rleungx commented Jul 6, 2022

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

nolouch commented Jul 25, 2022

LLThomas commented Aug 1, 2022

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

ti-chi-bot commented Oct 10, 2022

LLThomas commented Jul 6, 2022 •

edited

Loading

ti-chi-bot commented Jul 6, 2022 •

edited

Loading

codecov bot commented Jul 6, 2022 •

edited

Loading