schedule, server: persist scheduler list to etcd #769

Connor1996 · 2017-09-26T08:57:21Z

persist scheduler list to etcd to make sure the schedulers added or removed in dynamic way can still be recreated after leader change or pd-server restart.

…e empty in clone() of ReplicationConfig

siddontang · 2017-09-26T09:16:20Z

conf/config.toml

-# if empty, it will use balanceLeader, balanceRegion, hotReadRegion, hotWriteRegion as default
-[schedule.schedulers]
-# [schedule.schedulers.balanceLeader]
+# if empty, it will use balance-leader, balance-region, hot-region as default


please use two PRs, one is to change the name, the other is to persist the config.

siddontang · 2017-09-27T01:19:40Z

make.out

@@ -0,0 +1 @@
+GOPATH=/Users/Connor/Coding/src/github.com/pingcap/pd/_vendor go build -ldflags '-X "github.com/pingcap/pd/server.PDBuildTS=2017-09-26 08:17:22" -X "github.com/pingcap/pd/server.PDGitHash=d9704080e36f0adde864ffaac24a11b3cc9f0a1c" -X "github.com/pingcap/pd/server.PDGitBranch=connor/persist-scheduler"' -o bin/pd-server cmd/pd-server/main.go


remove this file

siddontang · 2017-09-27T01:20:37Z

I suggest changing the name at first, then support persistence.

Connor1996 · 2017-09-27T03:05:27Z

OK, i will pull a new request for changing the name @siddontang

disksing · 2017-09-28T05:33:13Z

server/config.go

+	for _, schedulerCfg := range v.Schedulers {
+		// comparing args is to cover the case that there are schedulers in same type but not with same name
+		// such as two schedulers of type evictLeader,
+		// one name is "evict-leader-scheduler-1" and the other is "evict-leader-scheduler-2"


Why not just compare their names?

Assuming that we are going to add two schedulers, "evict-leader-scheduler-1" and "evict-leader-scheduler-2", but when creating them we should pass "evit-leader" to CreateScheduler() not their name. And schedulers reloading will call CreateScheduler(), thus the name store in schedulerCfg is their type "evit-leader", not their name "evict-leader-scheduler-1" or "evict-leader-scheduler-2". And for here, "evit-leader" is not enough to distinguish "evict-leader-scheduler-1" with "evict-leader-scheduler-2"

disksing · 2017-09-28T07:22:14Z

server/config.go

+		// one name is "evict-leader-scheduler-1" and the other is "evict-leader-scheduler-2"
+		if schedulerCfg.Type == tp && stringSliceEqual(schedulerCfg.Args, args) {
+			return false
+		}


How about add a Equals method to SchedulerConfig?

disksing

The rest LGTM.

disksing · 2017-09-28T09:17:58Z

server/config.go

+			return false
+		}
+		if (a == nil) != (b == nil) {
+			return false


I think previous check already implicitly guaranteed it is true.

nolouch · 2017-09-28T09:42:31Z

server/coordinator.go

@@ -270,6 +270,9 @@ func (c *coordinator) addScheduler(scheduler schedule.Scheduler, interval time.D
 	c.wg.Add(1)
 	go c.runScheduler(s)
 	c.schedulers[s.GetName()] = s
+	if c.opt.AddSchedulerCfg(s.GetType(), args) {
+		c.opt.persist(c.kv)


print a log if persist meet error

nolouch · 2017-09-28T09:45:54Z

server/schedulers/balance_leader.go

@@ -51,6 +51,10 @@ func (l *balanceLeaderScheduler) GetName() string {
 	return "balance-leader-scheduler"
 }

+func (l *balanceLeaderScheduler) GetType() string {
+	return "balanceLeader"


use kebab-case.

siddontang · 2017-09-29T01:49:53Z

conf/config.toml

-# [schedule.schedulers.balance-leader]
+# [[schedule.schedulers]]
+# type = "balance-leader"
+# args = ["foo", "bar"]


bad arg name example, what does foo or bar mean here?

nothing, just placeholder

Use more meaningful argument names if possible or remove these.

siddontang · 2017-09-29T01:52:38Z

server/config.go

+
+// Equals is to compare whether two ScheduleConfig is same or not
+func (c *SchedulerConfig) Equals(s SchedulerConfig) bool {
+	stringSliceEqual := func(a, b []string) bool {


use reflect.DeepEqual

nolouch

LGTM

siddontang · 2017-09-29T08:42:27Z

server/config.go

-	"balance-region": toml.Primitive{},
-	"balance-leader": toml.Primitive{},
-	"hot-region":     toml.Primitive{},
+	SchedulerConfig{Type: "balance-region"},


seem you can use

SchedulerConfigs { {Type:"xxx"}, {Type:"xxx"}, }

siddontang · 2017-09-30T02:49:42Z

PTAL @disksing

siddontang · 2017-09-30T02:49:47Z

LGTM

nolouch · 2017-09-30T03:50:04Z

server/coordinator.go

 		if err != nil {
-			log.Errorf("can not create scheduler %s: %v", name, err)
+			log.Errorf("can not create scheduler %s: %v", schedulerCfg.Type, err)


maybe you can remove invalid persist schedulers in here according to the error types.

disksing · 2017-09-30T07:36:05Z

LGTM

nolouch

LGTM

This reverts commit 16e898c.

Connor1996 added 5 commits September 26, 2017 16:06

schudule, server: persist scheduler list to etcd

afd7bd1

server: fix bug that use copy() incorrectly leading to src slice to b…

43a712e

…e empty in clone() of ReplicationConfig

add test of persist scheduler

c1e395b

fix dead lock in mustNewCluster

d040b5a

abandon test of persist-scheduler, amend test of add scheduler

305c92a

Connor1996 force-pushed the connor/persist-scheduler branch from c52f5b3 to da092b3 Compare September 26, 2017 09:10

siddontang reviewed Sep 26, 2017

View reviewed changes

Connor1996 force-pushed the connor/persist-scheduler branch from da092b3 to 35beed9 Compare September 26, 2017 10:58

siddontang reviewed Sep 27, 2017

View reviewed changes

merge master

349d872

Connor1996 force-pushed the connor/persist-scheduler branch from e6b198a to 349d872 Compare September 27, 2017 03:12

merge master, use in-memory kvBase for testing

40f88ca

disksing reviewed Sep 28, 2017

View reviewed changes

add Equals method of SchedulerConfig

20445fc

disksing reviewed Sep 28, 2017

View reviewed changes

Connor1996 added 3 commits September 28, 2017 17:25

clean verbose code

26c78b7

add comment

99c1147

Merge branch 'master' into connor/persist-scheduler

81902c9

nolouch reviewed Sep 28, 2017

View reviewed changes

Connor1996 added 3 commits September 28, 2017 18:00

fix typo

9574357

add log

e295ebe

add test persist-scheduler

a85bee5

siddontang reviewed Sep 29, 2017

View reviewed changes

Connor1996 force-pushed the connor/persist-scheduler branch from aa6d4fc to 5f70e77 Compare September 29, 2017 03:19

use reflect.DeepEqual instead

dacd8c7

Connor1996 force-pushed the connor/persist-scheduler branch from 5f70e77 to dacd8c7 Compare September 29, 2017 05:00

nolouch approved these changes Sep 29, 2017

View reviewed changes

siddontang reviewed Sep 29, 2017

View reviewed changes

Connor1996 added 2 commits September 29, 2017 20:48

improve test of persist-scheduler and add log

c56aa0e

use meanful argument in config

79ab497

auto delete invalid scheduler config

8bf0508

Connor1996 force-pushed the connor/persist-scheduler branch from 2900eb8 to 8bf0508 Compare September 30, 2017 03:23

Merge branch 'master' into connor/persist-scheduler

769dfc6

nolouch reviewed Sep 30, 2017

View reviewed changes

avoid persist too frequently

3bea025

nolouch approved these changes Sep 30, 2017

View reviewed changes

Connor1996 merged commit 16e898c into master Sep 30, 2017

Connor1996 deleted the connor/persist-scheduler branch September 30, 2017 08:13

Connor1996 mentioned this pull request Sep 30, 2017

create schedulers base on configuration. #738

Closed

disksing added a commit that referenced this pull request Oct 7, 2017

Revert "schedule, server: persist scheduler list to etcd (#769)"

c54c5de

This reverts commit 16e898c.

disksing added a commit that referenced this pull request Oct 7, 2017

Revert "schedule, server: persist scheduler list to etcd (#769)" (#784)

3947a95

This reverts commit 16e898c.

Connor1996 mentioned this pull request Oct 9, 2017

sccheduler, server: persist scheudler list to etcd #785

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schedule, server: persist scheduler list to etcd #769

schedule, server: persist scheduler list to etcd #769

Connor1996 commented Sep 26, 2017 •

edited

siddontang Sep 26, 2017

siddontang Sep 27, 2017

siddontang commented Sep 27, 2017

Connor1996 commented Sep 27, 2017

disksing Sep 28, 2017

Connor1996 Sep 28, 2017

disksing Sep 28, 2017

Connor1996 Sep 28, 2017

disksing left a comment

disksing Sep 28, 2017

nolouch Sep 28, 2017

nolouch Sep 28, 2017

siddontang Sep 29, 2017

Connor1996 Sep 29, 2017

siddontang Sep 29, 2017

siddontang Sep 29, 2017

nolouch left a comment

siddontang Sep 29, 2017

siddontang commented Sep 30, 2017

siddontang commented Sep 30, 2017

nolouch Sep 30, 2017

disksing commented Sep 30, 2017

nolouch left a comment

		@@ -0,0 +1 @@
		GOPATH=/Users/Connor/Coding/src/github.com/pingcap/pd/_vendor go build -ldflags '-X "github.com/pingcap/pd/server.PDBuildTS=2017-09-26 08:17:22" -X "github.com/pingcap/pd/server.PDGitHash=d9704080e36f0adde864ffaac24a11b3cc9f0a1c" -X "github.com/pingcap/pd/server.PDGitBranch=connor/persist-scheduler"' -o bin/pd-server cmd/pd-server/main.go

schedule, server: persist scheduler list to etcd #769

schedule, server: persist scheduler list to etcd #769

Conversation

Connor1996 commented Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Sep 27, 2017

Connor1996 commented Sep 27, 2017

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

disksing 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

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Sep 30, 2017

siddontang commented Sep 30, 2017

Choose a reason for hiding this comment

disksing commented Sep 30, 2017

nolouch left a comment

Choose a reason for hiding this comment

Connor1996 commented Sep 26, 2017 •

edited