*: region filter #5135

CabinfeverB · 2022-06-09T03:56:21Z

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

Issue Number: ref #5257

What is changed and how does it work?

remove RegionOption, and add RegionFilter

remove `RegionOption`, and add `RegionFilter`

Check List

Tests

Unit test

Release note

 None.

ti-chi-bot · 2022-06-09T03:56:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AndreMouche
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.

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

codecov · 2022-06-09T06:20:21Z

Codecov Report

Merging #5135 (987a1fa) into master (8ab5c16) will decrease coverage by 0.02%.
The diff coverage is 80.11%.

@@            Coverage Diff             @@
##           master    #5135      +/-   ##
==========================================
- Coverage   75.69%   75.66%   -0.03%     
==========================================
  Files         311      312       +1     
  Lines       30909    30962      +53     
==========================================
+ Hits        23396    23427      +31     
- Misses       5503     5523      +20     
- Partials     2010     2012       +2

Flag	Coverage Δ
unittests	`75.66% <80.11%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
server/core/region_option.go	`83.17% <ø> (ø)`
server/handler.go	`52.58% <0.00%> (ø)`
server/schedule/plan/status.go	`77.77% <ø> (ø)`
server/schedulers/hot_region.go	`84.17% <0.00%> (+0.25%)`	⬆️
server/storage/endpoint/meta.go	`63.63% <ø> (-0.41%)`	⬇️
server/schedule/checker/merge_checker.go	`69.69% <50.00%> (ø)`
server/storage/storage.go	`71.15% <50.00%> (+0.88%)`	⬆️
server/region_syncer/server.go	`87.36% <66.66%> (+4.21%)`	⬆️
server/server.go	`74.76% <71.42%> (+0.62%)`	⬆️
server/schedule/filter/region_filters.go	`77.41% <77.41%> (ø)`
... and 42 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 6aaff24...987a1fa. Read the comment docs.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> --wip-- [skip ci]

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-13T08:37:08Z

server/schedule/healthy.go

 )

 // IsRegionHealthy checks if a region is healthy for scheduling. It requires the
 // region does not have any down or pending peers.
 func IsRegionHealthy(region *core.RegionInfo) bool {
-	return IsRegionHealthyAllowPending(region) && len(region.GetPendingPeers()) == 0
+	pendingFilter := filter.NewRegionPengdingFilter("option")


Why not keep them as it is?

rleungx · 2022-07-13T08:41:16Z

server/schedule/filter/region_filters.go

+}
+
+// Cluster provides an overview of a cluster's regions distribution.
+type Cluster interface {


Can we avoid defining this interface?

I defined this because of avoiding import cycles

defining this interface looks ok to me, However could we make the interface private?

AndreMouche · 2022-07-13T09:42:01Z

server/schedule/filter/filters.go

@@ -137,7 +137,7 @@ func Target(opt *config.PersistOptions, store *core.StoreInfo, filters []Filter)
 	storeAddress := store.GetAddress()
 	storeID := strconv.FormatUint(store.GetID(), 10)
 	for _, filter := range filters {
-		if !filter.Target(opt, store).IsOK() {
+		if filter.Target(opt, store) != statusStoreOK {


Could we use IsOK?

AndreMouche · 2022-07-13T09:43:54Z

server/schedule/filter/filters.go

-	statusPauseLeader        = plan.NewStatus(plan.StatusStoreBlocked, "the store is not allowed to transfer leader, there might be an evict-leader-scheduler")
-	statusRejectLeader       = plan.NewStatus(plan.StatusStoreBlocked, "the store is not allowed to transfer leader, please check 'label-property'")
-	statusSlowStore          = plan.NewStatus(plan.StatusStoreBlocked, "the store is slow and are evicting leaders, there might be an evict-slow-store-scheduler")
+	statusStoreOK                 = plan.NewStatus(plan.StatusOK)


Could we merge statusStoreOK with statusRegionOK?
How about defining a single file status.go to store these status variables?

It seems that only statusOK needs a single file, is it necessary?

How about put all status into that file?

AndreMouche · 2022-07-13T09:55:56Z

server/schedule/filter/region_filters.go

+	return statusRegionOK
+}
+
+// isEmptyRegionAllowBalance checks if a region is an empty region and can be balanced.


Suggested change

// isEmptyRegionAllowBalance checks if a region is an empty region and can be balanced.

// isEmptyRegionAllowBalance returns true if the region is not empty or the number of regions is too small.

AndreMouche · 2022-07-13T09:58:04Z

server/schedule/filter/region_filters.go

+}
+
+// Cluster provides an overview of a cluster's regions distribution.
+type Cluster interface {


defining this interface looks ok to me, However could we make the interface private?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-14T04:12:07Z

server/schedule/healthy.go

-		return cluster.GetRuleManager().FitRegion(cluster, region).IsSatisfied()
-	}
-	return len(region.GetLearners()) == 0 && len(region.GetPeers()) == cluster.GetOpts().GetMaxReplicas()
+	filter := filter.NewRegionReplicatedFilter("option", cluster)


I think Replicated is a more complex logic than PendingPeer or DownPeer, so I only want to maintain it in one place

rleungx · 2022-07-14T04:13:59Z

server/schedule/filter/candidates_test.go

 	}
-	return statusNoNeed
+	return statusStoreNoNeed


How about keep using statusNoNeed?

rleungx · 2022-07-14T04:17:55Z

server/schedule/filter/region_filters.go

+}
+
+// cluster provides an overview of a cluster's regions distribution.
+type regionFilterCluster interface {


seem that some interfaces are not used

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-14T09:21:10Z

server/schedule/filter/healthy.go

+	return hasDownPeers(region)
+}
+
+func hasPendingPeers(region *core.RegionInfo) bool {


The name doesn't match the implementation.

rleungx · 2022-07-14T09:21:15Z

server/schedule/filter/healthy.go

+	return len(region.GetPendingPeers()) == 0
+}
+
+func hasDownPeers(region *core.RegionInfo) bool {


AndreMouche

LGTM

rleungx · 2022-07-14T09:25:40Z

server/schedule/filter/region_filters.go

+		return statusOK
+	}
+	if !isRegionReplicasSatisfied(f.cluster, region) {
+		return statusRegionIsolation


Why is it related to isolation?

rleungx · 2022-07-14T09:26:14Z

server/schedule/filter/region_filters.go

+// RegionFilter is an interface to filter region.
+type RegionFilter interface {
+	// RegionFilter is used to indicate where the filter will act on.
+	Scope() string


Is it needed?

rleungx · 2022-07-14T09:32:31Z

server/schedule/filter/status.go

+
+import "github.com/tikv/pd/server/schedule/plan"
+
+var (


Why not put them together if you use a separate file to define the status?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-14T10:24:39Z

server/schedule/plan/status.go

@@ -46,6 +46,8 @@ const (
 	StatusRegionUnhealthy
 	// StatusRegionEmpty represents the region cannot be selected due to the region is empty.
 	StatusRegionEmpty
+	// StatusRegionUnReplicated represents the region does not have enough replicas.
+	StatusRegionUnReplicated


Suggested change

StatusRegionUnReplicated

StatusRegionNotReplicated

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx

The rest LGTM.

rleungx · 2022-07-14T10:56:07Z

server/schedule/filter/filters.go

@@ -505,17 +483,17 @@ func (f *StoreStateFilter) Source(opts *config.PersistOptions, store *core.Store
 // target.
 func (f *StoreStateFilter) Target(opts *config.PersistOptions, store *core.StoreInfo) (status plan.Status) {
 	if f.TransferLeader {
-		if status = f.anyConditionMatch(leaderTarget, opts, store); !status.IsOK() {


Why change it back?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-07-15T03:10:52Z

/merge

ti-chi-bot · 2022-07-15T03:10:53Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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.

ti-chi-bot · 2022-07-15T03:10:54Z

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

Commit hash: 987a1fa

ti-chi-bot added do-not-merge/needs-linked-issue release-note The PR should write the release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 9, 2022

ti-chi-bot requested review from lhy1024 and nolouch June 9, 2022 03:56

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

rleungx mentioned this pull request Jul 4, 2022

Tracking issue for the scheduling diagnosis #5257

Open

5 tasks

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

CabinfeverB marked this pull request as ready for review July 5, 2022 09:50

ti-chi-bot added release-note-none and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue release-note The PR should write the release note. labels Jul 5, 2022

CabinfeverB requested review from rleungx and AndreMouche and removed request for nolouch and lhy1024 July 5, 2022 09:52

CabinfeverB changed the title ~~region filter~~ *: region filter Jul 5, 2022

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

CabinfeverB added 6 commits July 13, 2022 15:06

region filter

61fd5ee

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

region filter

3053b3a

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

region filter

be4cd0d

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

region filter

798f231

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

remove useless

93cce23

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add test

b6d6dc5

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB force-pushed the region_filter branch from aaca3b8 to 73b48dd Compare July 13, 2022 07:31

CabinfeverB added 2 commits July 13, 2022 15:44

fix typo

2d3204e

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> --wip-- [skip ci]

merge status

a4cb6b3

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge master

bf21523

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB force-pushed the region_filter branch from 73b48dd to bf21523 Compare July 13, 2022 07:45

rleungx reviewed Jul 13, 2022

View reviewed changes

AndreMouche reviewed Jul 13, 2022

View reviewed changes

address the comment

322c3b2

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

rleungx reviewed Jul 14, 2022

View reviewed changes

CabinfeverB added 2 commits July 14, 2022 16:18

address the comment

56aa9ac

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address the comment

2e4d7f3

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jul 14, 2022

View reviewed changes

AndreMouche approved these changes Jul 14, 2022

View reviewed changes

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

rleungx reviewed Jul 14, 2022

View reviewed changes

address the comment

485c98a

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jul 14, 2022

View reviewed changes

address the comment

cb464e5

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx approved these changes Jul 14, 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 Jul 14, 2022

CabinfeverB added 4 commits July 14, 2022 18:58

address the comment

24867f7

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address the comment

94860ac

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address the comment

20937fc

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into region_filter

987a1fa

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

ti-chi-bot merged commit 05444e2 into tikv:master Jul 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: region filter #5135

*: region filter #5135

CabinfeverB commented Jun 9, 2022 •

edited

ti-chi-bot commented Jun 9, 2022 •

edited

codecov bot commented Jun 9, 2022 •

edited

rleungx Jul 13, 2022

rleungx Jul 13, 2022

CabinfeverB Jul 13, 2022

AndreMouche Jul 13, 2022

AndreMouche Jul 13, 2022

AndreMouche Jul 13, 2022

CabinfeverB Jul 13, 2022

AndreMouche Jul 14, 2022

AndreMouche Jul 13, 2022

AndreMouche Jul 13, 2022

rleungx Jul 14, 2022

CabinfeverB Jul 14, 2022

rleungx Jul 14, 2022

rleungx Jul 14, 2022

CabinfeverB Jul 14, 2022

rleungx Jul 14, 2022

rleungx Jul 14, 2022

AndreMouche left a comment

rleungx Jul 14, 2022

rleungx Jul 14, 2022

rleungx Jul 14, 2022

rleungx Jul 14, 2022

rleungx left a comment

rleungx Jul 14, 2022

rleungx commented Jul 15, 2022

ti-chi-bot commented Jul 15, 2022

ti-chi-bot commented Jul 15, 2022

	// isEmptyRegionAllowBalance checks if a region is an empty region and can be balanced.
	// isEmptyRegionAllowBalance returns true if the region is not empty or the number of regions is too small.

*: region filter #5135

*: region filter #5135

Conversation

CabinfeverB commented Jun 9, 2022 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Jun 9, 2022 • edited

codecov bot commented Jun 9, 2022 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreMouche 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

rleungx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Jul 15, 2022

ti-chi-bot commented Jul 15, 2022

ti-chi-bot commented Jul 15, 2022

CabinfeverB commented Jun 9, 2022 •

edited

ti-chi-bot commented Jun 9, 2022 •

edited

codecov bot commented Jun 9, 2022 •

edited