schedule: move the logic of patrol region from coordinator to checkers #8428

lhy1024 · 2024-07-22T16:16:02Z

What problem does this PR solve?

Issue Number: Ref #7963

What is changed and how does it work?

move the logic of patrol region from coordinator to checkers
introduce PatrolRegionContext
modify some comments

Check List

Tests

Unit test

Release note

None.

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2024-07-23T02:58:01Z

/test pull-integration-realcluster-test

Signed-off-by: lhy1024 <[email protected]>

codecov · 2024-07-25T06:54:50Z

Codecov Report

Attention: Patch coverage is 92.75362% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.36%. Comparing base (45dac53) to head (24a026b).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8428      +/-   ##
==========================================
- Coverage   77.40%   77.36%   -0.05%     
==========================================
  Files         472      472              
  Lines       61796    61811      +15     
==========================================
- Hits        47836    47820      -16     
- Misses      10382    10430      +48     
+ Partials     3578     3561      -17

Flag	Coverage Δ
unittests	`77.36% <92.75%> (-0.05%)`	⬇️

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

HuSharp

rest LGTM

pkg/schedule/checker/checker_controller.go

Signed-off-by: lhy1024 <[email protected]>

rleungx · 2024-07-31T07:23:27Z

pkg/schedule/checker/checker_controller.go

@@ -233,3 +417,39 @@ func (c *Controller) GetPauseController(name string) (*PauseController, error) {
 		return nil, errs.ErrCheckerNotFound.FastGenByArgs()
 	}
 }
+
+// PatrolRegionContext is used to store the context of patrol regions.


Do we still need it?

it will be used in the next pr, it may be more suitable to move it to the next pr?

Should the fields be part of the checker controller? Is it necessary to wrap it?

I think it is needed. It will contain more value and config in the future.

pd/pkg/schedule/checker/checker_controller.go

Lines 465 to 479 in ae0778f

type PatrolRegionContext struct {

syncutil.RWMutex

// config

interval time.Duration

workerCount int

scanLimit int

// status

patrolRoundStartTime time.Time

duration time.Duration

// workers

workersCtx context.Context

workersCancel context.CancelFunc

regionChan chan *core.RegionInfo

wg sync.WaitGroup

}

rleungx · 2024-07-31T09:11:36Z

pkg/schedule/checker/checker_controller.go

+// PatrolRegions is used to scan regions.
+// The checkers will check these regions to decide if they need to do some operations.
+func (c *Controller) PatrolRegions() {
+	c.patrolRegionContext.init(c.cluster)


Suggested change

c.patrolRegionContext.init(c.cluster)

patrolInterval := cluster.GetCheckerConfig().GetPatrolRegionInterval()

c.patrolRegionContext.init(patrolInterval)

c.cluster is needed in updateTickerIfNeeded

init doesn't need it.

rleungx · 2024-07-31T09:16:43Z

pkg/schedule/checker/checker_controller.go

+func (c *Controller) updateTickerIfNeeded(ticker *time.Ticker) {
+	// Note: we reset the ticker here to support updating configuration dynamically.
+	newInterval := c.cluster.GetCheckerConfig().GetPatrolRegionInterval()
+	if c.patrolRegionContext.interval != newInterval {


Why not keep it the same as before, just reset it every time?

Maybe move it to patrolRegionContext?

pkg/schedule/checker/checker_controller.go

rleungx · 2024-07-31T09:18:37Z

pkg/schedule/checker/checker_controller.go

+	interval time.Duration
+	// status
+	patrolRoundStartTime time.Time


These fields are unnecessary IMO.

I tried to add a new commit, what do you think? Is it better to move both updateTickerIfNeeded and roundUpdateMetrics to the patrolRegion function, would that make it too long?

ti-chi-bot · 2024-07-31T09:21:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuSharp, rleungx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [HuSharp,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-31T09:21:13Z

[LGTM Timeline notifier]

Timeline:

2024-07-26 06:55:30.170099626 +0000 UTC m=+1200952.161041094: ☑️ agreed by HuSharp.
2024-07-31 09:21:12.551101229 +0000 UTC m=+347588.831149295: ☑️ agreed by rleungx.

rleungx · 2024-07-31T09:21:38Z

/hold

lhy1024 · 2024-07-31T09:56:29Z

/test pull-integration-realcluster-test

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2024-07-31T10:24:19Z

/test pull-integration-realcluster-test

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2024-08-01T03:26:48Z

/retest-required

rleungx · 2024-08-01T03:45:32Z

/hold cancel

schedule: move the logic of patrol region from coordinator to checkers

f6d853e

Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 22, 2024

lhy1024 added 2 commits July 25, 2024 14:47

fix test

3141e03

Signed-off-by: lhy1024 <[email protected]>

Merge branch 'master' of github.com:tikv/pd into refactor-coordinator

2a45319

HuSharp reviewed Jul 26, 2024

View reviewed changes

pkg/schedule/checker/checker_controller.go Show resolved Hide resolved

pkg/schedule/checker/checker_controller.go Outdated Show resolved Hide resolved

address comments with Camel case

2b2570a

Signed-off-by: lhy1024 <[email protected]>

HuSharp approved these changes Jul 26, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 26, 2024

rleungx reviewed Jul 31, 2024

View reviewed changes

rleungx approved these changes Jul 31, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 31, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2024

move ticker to PatrolRegionContext

9784aac

Signed-off-by: lhy1024 <[email protected]>

lhy1024 force-pushed the refactor-coordinator branch from 339d545 to 9784aac Compare July 31, 2024 10:12

address comments

f31036f

Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2024

Merge branch 'master' into refactor-coordinator

24a026b

ti-chi-bot bot merged commit 3ce31ef into tikv:master Aug 1, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schedule: move the logic of patrol region from coordinator to checkers #8428

schedule: move the logic of patrol region from coordinator to checkers #8428

lhy1024 commented Jul 22, 2024 •

edited

Loading

lhy1024 commented Jul 23, 2024

codecov bot commented Jul 25, 2024 •

edited

Loading

HuSharp left a comment

rleungx Jul 31, 2024

lhy1024 Jul 31, 2024

rleungx Jul 31, 2024

lhy1024 Jul 31, 2024 •

edited

Loading

rleungx Jul 31, 2024

lhy1024 Jul 31, 2024

rleungx Jul 31, 2024 •

edited

Loading

rleungx Jul 31, 2024

lhy1024 Jul 31, 2024

rleungx Jul 31, 2024

lhy1024 Jul 31, 2024

ti-chi-bot bot commented Jul 31, 2024

ti-chi-bot bot commented Jul 31, 2024

rleungx commented Jul 31, 2024

lhy1024 commented Jul 31, 2024

lhy1024 commented Jul 31, 2024

lhy1024 commented Aug 1, 2024

rleungx commented Aug 1, 2024

	type PatrolRegionContext struct {
	syncutil.RWMutex
	// config
	interval time.Duration
	workerCount int
	scanLimit int
	// status
	patrolRoundStartTime time.Time
	duration time.Duration
	// workers
	workersCtx context.Context
	workersCancel context.CancelFunc
	regionChan chan *core.RegionInfo
	wg sync.WaitGroup
	}

	c.patrolRegionContext.init(c.cluster)
	patrolInterval := cluster.GetCheckerConfig().GetPatrolRegionInterval()
	c.patrolRegionContext.init(patrolInterval)

schedule: move the logic of patrol region from coordinator to checkers #8428

schedule: move the logic of patrol region from coordinator to checkers #8428

Conversation

lhy1024 commented Jul 22, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

lhy1024 commented Jul 23, 2024

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

HuSharp 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

lhy1024 Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx Jul 31, 2024 • edited Loading

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 bot commented Jul 31, 2024

ti-chi-bot bot commented Jul 31, 2024

[LGTM Timeline notifier]

rleungx commented Jul 31, 2024

lhy1024 commented Jul 31, 2024

lhy1024 commented Jul 31, 2024

lhy1024 commented Aug 1, 2024

rleungx commented Aug 1, 2024

lhy1024 commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

lhy1024 Jul 31, 2024 •

edited

Loading

rleungx Jul 31, 2024 •

edited

Loading