scheduler: extract storage operations to schedulerConfig interface #8515

okJiang · 2024-08-12T03:30:48Z

What problem does this PR solve?

Issue Number: Close #8514, ref #8379

What is changed and how does it work?

Check List

Tests

Unit test
Integration test

Code changes

no

Side effects

no

Related changes

no

Release note

None.

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot · 2024-08-12T03:30:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: okJiang <819421878@qq.com>

codecov · 2024-08-12T03:54:16Z

Codecov Report

Attention: Patch coverage is 82.35294% with 24 lines in your changes missing coverage. Please review.

Project coverage is 77.54%. Comparing base (1c1cd99) to head (4ba7249).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8515      +/-   ##
==========================================
+ Coverage   77.40%   77.54%   +0.14%     
==========================================
  Files         473      474       +1     
  Lines       61934    61864      -70     
==========================================
+ Hits        47941    47975      +34     
+ Misses      10409    10356      -53     
+ Partials     3584     3533      -51

Flag	Coverage Δ
unittests	`77.54% <82.35%> (+0.14%)`	⬆️

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

Signed-off-by: okJiang <819421878@qq.com>

okJiang · 2024-08-12T07:33:21Z

/test pull-integration-realcluster-test

okJiang

self review

pkg/schedule/schedulers/balance_leader.go

pkg/schedule/schedulers/evict_leader.go

pkg/schedule/schedulers/evict_slow_store.go

Signed-off-by: okJiang <819421878@qq.com>

lhy1024

Is it similar to #7598?

okJiang · 2024-08-12T08:38:54Z

Is it similar to #7598?

yes, #7598 wrapped persist only, #8515 wrapped persist and load

Signed-off-by: okJiang <819421878@qq.com>

…nify-scheduler-config

Signed-off-by: okJiang <819421878@qq.com>

okJiang · 2024-08-13T06:46:23Z

Could you please take a look? @lhy1024 @rleungx

rleungx · 2024-08-13T07:35:44Z

pkg/schedule/schedulers/split_bucket.go

@@ -120,7 +112,7 @@ func (h *splitBucketHandler) updateConfig(w http.ResponseWriter, r *http.Request
 	}
 	newc, _ := json.Marshal(h.conf)
 	if !bytes.Equal(oldc, newc) {
-		if err := h.conf.persistLocked(); err != nil {
+		if err := h.conf.save(h.conf); err != nil {


It looks strange.

lhy1024 · 2024-08-13T07:39:09Z

pkg/schedule/schedulers/balance_leader.go

@@ -79,7 +79,7 @@ func (conf *balanceLeaderSchedulerConfig) update(data []byte) (int, any) {
 			}
 			return http.StatusBadRequest, "invalid batch size which should be an integer between 1 and 10"
 		}
-		if err := conf.persistLocked(); err != nil {
+		if err := conf.save(conf); err != nil {


how about conf.save()

If we change it to the form of conf.save(), then we will have to save conf(xxxxxSchedulerConfig) as a pointer(any) in baseSchedulerConfig. I'm not sure if this is a good idea. If everyone agrees, I can make this change.

cc @rleungx

I've made this change, we can compare them and choose the best one. 96ef87e

Actually, I also think that save() is better👀

lhy1024 · 2024-08-13T07:44:03Z

pkg/schedule/schedulers/evict_slow_trend.go

 	newCfg := &evictSlowTrendSchedulerConfig{}
-	if err = DecodeConfig([]byte(cfgData), newCfg); err != nil {
+	if err := s.conf.load(newCfg); err != nil {


How about err, newCfg := s.conf.load()

We need to know the structures that need to be decoded, so we have to input them in.

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot · 2024-08-14T02:40:08Z

[LGTM Timeline notifier]

Timeline:

2024-08-13 07:35:53.904483225 +0000 UTC m=+253438.607952860: ☑️ agreed by rleungx.
2024-08-14 02:40:07.426701801 +0000 UTC m=+322092.130171442: ☑️ agreed by lhy1024.

niubell · 2024-08-14T02:55:16Z

pkg/schedule/schedulers/evict_slow_store.go

-	defer handler.config.Unlock()
-	prevRecoveryDurationGap := handler.config.RecoveryDurationGap
+
+	conf := handler.config


Is the conf assignment also need to be in the lock scope?

This is a pointer, so this assignment won't trigger any data reading or writing.

But to avoid similar misunderstandings, I'll revert this change, it won't affect this pull request.

niubell · 2024-08-14T02:56:19Z

pkg/schedule/schedulers/evict_slow_trend.go

-	handler.config.Lock()
-	defer handler.config.Unlock()
-	prevRecoveryDurationGap := handler.config.RecoveryDurationGap
+	conf := handler.config


Signed-off-by: okJiang <819421878@qq.com>

…nify-scheduler-config

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot · 2024-08-14T03:46:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lhy1024, niubell, 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~~ [lhy1024,rleungx]
~~pkg/schedule/schedulers/OWNERS~~ [niubell]

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

exact schedulerConfig interface

b3ed767

Signed-off-by: okJiang <819421878@qq.com>

optimize

1483339

Signed-off-by: okJiang <819421878@qq.com>

okJiang changed the title ~~scheduler: extract storage operator to schedulerConfig interface~~ scheduler: extract storage operations to schedulerConfig interface Aug 12, 2024

okJiang marked this pull request as ready for review August 12, 2024 05:01

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2024

okJiang added 3 commits August 12, 2024 13:11

add an ut

c6267ff

Signed-off-by: okJiang <819421878@qq.com>

add more ut

b2fc257

Signed-off-by: okJiang <819421878@qq.com>

revert a commit

2498970

Signed-off-by: okJiang <819421878@qq.com>

This was referenced Aug 12, 2024

Standardize the use of scheduler name/type and checker name/type #8379

Closed

scheduler: remove old scheduler name #8516

Merged

okJiang commented Aug 12, 2024

View reviewed changes

pkg/schedule/schedulers/balance_leader.go Outdated Show resolved Hide resolved

pkg/schedule/schedulers/evict_leader.go Outdated Show resolved Hide resolved

pkg/schedule/schedulers/evict_slow_store.go Outdated Show resolved Hide resolved

okJiang added 2 commits August 12, 2024 16:14

fix comment

33b1c7c

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'master' into unify-scheduler-config

dc5f3ca

lhy1024 reviewed Aug 12, 2024

View reviewed changes

okJiang added 3 commits August 12, 2024 17:31

trigger ci

1dfc4e6

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'unify-scheduler-config' of github.com:okJiang/pd into u…

9c61f70

…nify-scheduler-config

trigger ci

5832835

Signed-off-by: okJiang <819421878@qq.com>

rleungx reviewed Aug 13, 2024

View reviewed changes

rleungx approved these changes Aug 13, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 13, 2024

lhy1024 reviewed Aug 13, 2024

View reviewed changes

okJiang added 2 commits August 13, 2024 16:56

fix comment: update save(conf) to save()

96ef87e

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'master' into unify-scheduler-config

4ba7249

lhy1024 approved these changes Aug 14, 2024

View reviewed changes

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

niubell reviewed Aug 14, 2024

View reviewed changes

okJiang added 4 commits August 14, 2024 11:11

fix comment

fa73a15

Signed-off-by: okJiang <819421878@qq.com>

Merge branch 'unify-scheduler-config' of github.com:okJiang/pd into u…

fa25ca0

…nify-scheduler-config

trigger ci

ef974fc

Signed-off-by: okJiang <819421878@qq.com>

trigger ci

0d73b58

Signed-off-by: okJiang <819421878@qq.com>

niubell approved these changes Aug 14, 2024

View reviewed changes

ti-chi-bot bot added the approved label Aug 14, 2024

ti-chi-bot bot merged commit 3b37572 into tikv:master Aug 14, 2024
21 checks passed

okJiang deleted the unify-scheduler-config branch August 14, 2024 03:46

lhy1024 mentioned this pull request Aug 14, 2024

scheduler: rafactor persist config #7598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: extract storage operations to schedulerConfig interface #8515

scheduler: extract storage operations to schedulerConfig interface #8515

okJiang commented Aug 12, 2024

ti-chi-bot bot commented Aug 12, 2024

codecov bot commented Aug 12, 2024 •

edited

Loading

okJiang commented Aug 12, 2024

okJiang left a comment

lhy1024 left a comment

okJiang commented Aug 12, 2024

okJiang commented Aug 13, 2024

rleungx Aug 13, 2024

lhy1024 Aug 13, 2024

okJiang Aug 13, 2024 •

edited

Loading

okJiang Aug 13, 2024

lhy1024 Aug 13, 2024

okJiang Aug 13, 2024

ti-chi-bot bot commented Aug 14, 2024

niubell Aug 14, 2024

okJiang Aug 14, 2024

niubell Aug 14, 2024

ti-chi-bot bot commented Aug 14, 2024

scheduler: extract storage operations to schedulerConfig interface #8515

scheduler: extract storage operations to schedulerConfig interface #8515

Conversation

okJiang commented Aug 12, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 12, 2024

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

okJiang commented Aug 12, 2024

okJiang left a comment

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

okJiang commented Aug 12, 2024

okJiang commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Aug 13, 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

ti-chi-bot bot commented Aug 14, 2024

[LGTM Timeline notifier]

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 Aug 14, 2024

codecov bot commented Aug 12, 2024 •

edited

Loading

okJiang Aug 13, 2024 •

edited

Loading