api: change store limit by label #2697

ZenoTan · 2020-07-29T08:06:12Z

What problem does this PR solve?

Solve issue #2568
ref: #2504

What is changed and how it works?

Support change store limit according to label. Label is treated as a special store rule here.
Command line like "store limit ... " can be used to change it.

Check List

Tests

Unit test
Integration test

Code changes

Has HTTP API interfaces change
Has pd-ctl interfaces change

Side effects

Related changes

Release note

ZenoTan · 2020-07-29T08:42:56Z

/run-all-tests

rleungx · 2020-07-31T06:02:39Z

tools/pd-ctl/pdctl/command/store_command.go

@@ -89,7 +89,7 @@ func NewSetStoreWeightCommand() *cobra.Command {
 // NewStoreLimitCommand returns a limit subcommand of storeCmd.
 func NewStoreLimitCommand() *cobra.Command {
 	c := &cobra.Command{
-		Use:   "limit [<type>]|[<store_id>|<all> <limit> <type>]",
+		Use:   "limit [<type>]|[<store_id>|<all>|<rule> <limit> <type>]",


Can we make the rule as an option?

You mean store limit (rule) (limit) (type)?

server/api/store.go

nolouch · 2020-08-03T04:14:51Z

server/config/config.go

+	// StoreRuleList is the history of store rules.
+	StoreRuleList StoreRules `toml:"store-rule-list" json:"store-rule-list"`
+	// StoreRuleLimit is the limit of scheduling for store rules.
+	StoreRuleLimit map[string]StoreLimitConfig `toml:"store-rule-limit" json:"store-rule-limit"`


How do we distinguish StoreRuleLimit and StoreLimit? I think it is a bit complicated.

Yes. If it is not exposed to users, then where to save the map(which is used when a new store comes)

Yes，we can hide him and provide a separate API for the query. cc @rleungx

Provide something like InternalConfig, which can be used in other scenarios like this. Some additional logic will be added to show config. Is it a good solution?

tests/pdctl/store/store_test.go

nolouch · 2020-08-04T07:59:31Z

server/api/router.go

@@ -110,6 +110,8 @@ func createRouter(ctx context.Context, prefix string, svr *server.Server) *mux.R
 	clusterRouter.HandleFunc("/stores/remove-tombstone", storesHandler.RemoveTombStone).Methods("DELETE")
 	clusterRouter.HandleFunc("/stores/limit", storesHandler.GetAllLimit).Methods("GET")
 	clusterRouter.HandleFunc("/stores/limit", storesHandler.SetAllLimit).Methods("POST")
+	clusterRouter.HandleFunc("/stores/rule/limit", storesHandler.GetRuleLimit).Methods("GET")
+	clusterRouter.HandleFunc("/stores/rule/limit", storesHandler.SetRuleLimit).Methods("POST")


I more likely change rule to group? how do you think? We already have a placement rule, this Rule may confuse users.

Actually, I prefer not to add a new API. we can make the rules as a parameter or option

nolouch · 2020-08-04T08:00:36Z

server/config/config.go

+	// StoreRuleList is the history of store rules.
+	StoreRuleList StoreRules `toml:"store-rule-list" json:"store-rule-list"`
+	// StoreRuleLimit is the limit of scheduling for store rules.
+	StoreRuleLimit map[string]StoreLimitConfig `toml:"store-rule-limit" json:"store-rule-limit"`


Yes，we can hide him and provide a separate API for the query. cc @rleungx

rleungx · 2020-08-25T09:31:09Z

server/config/config.go

+	valid  bool
+}
+
+func (sr *LabelStoreRule) parse(s string) {


Can we use a way like the store label API to parse the label?

rleungx · 2020-08-25T09:38:47Z

I think we can use the original command to set the store limit but provide an option to use the label. e.g. store limit add-peer 5 [<key> <value>, ...]. And for this label, we can filter the stores and directly set the specified value to the store which meeting the label requirement.

nolouch · 2020-09-01T05:40:58Z

Can we accelerate this PR? @ZenoTan

ZenoTan · 2020-09-03T03:52:13Z

Sure. Next week I'll work on this.

ZenoTan · 2020-09-07T03:27:00Z

Will open soon.

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

server/cluster/cluster.go

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

tools/pd-ctl/pdctl/command/store_command.go

nolouch · 2020-09-08T17:02:14Z

tests/pdctl/store/store_test.go

@@ -196,6 +196,14 @@ func (s *storeTestSuite) TestStore(c *C) {
 	limit2 = leaderServer.GetRaftCluster().GetStoreLimitByType(2, storelimit.RemovePeer)
 	c.Assert(limit2, Equals, float64(25))

+	// store limit all <key> <value> <rate> <type>
+	args = []string{"-u", pdAddr, "store", "limit", "all", "zone", "uk", "20", "remove-peer"}


can we add a simpler command for TiFlash？ cc @rleungx

Maybe store tiflash add-peer 60?

we can do it in another PR.

nolouch

LGTM

rleungx · 2020-09-10T01:12:40Z

tools/pd-ctl/pdctl/command/store_command.go

+			cmd.Println("Labels are an option of set all stores limit.")
+		} else {
+			postInput := map[string]interface{}{}
+			prefix := path.Join(storesPrefix, "limit")


How about using a constant?

rleungx · 2020-09-10T01:16:06Z

tests/pdctl/store/store_test.go

+	// store limit all <key> <value> <rate> <type>
+	args = []string{"-u", pdAddr, "store", "limit", "all", "zone", "uk", "20", "remove-peer"}
+	_, output, err = pdctl.ExecuteCommandC(cmd, args...)
+	c.Log(string(output))


Signed-off-by: ZenoTan <zenotan1998@gmail.com>

rleungx

LGTM

ZenoTan · 2020-09-10T04:01:28Z

/merge

ti-srebot · 2020-09-10T04:01:30Z

@ZenoTan Oops! auto merge is restricted to Committers of the SIG.See the corresponding SIG page for more information. Related SIG: scheduling(slack).

nolouch · 2020-09-10T05:49:48Z

/merge

ti-srebot · 2020-09-10T05:50:17Z

/run-all-tests

ti-srebot · 2020-09-10T06:20:49Z

@ZenoTan merge failed.

ZenoTan · 2020-09-10T06:29:06Z

/run-all-tests

ZenoTan · 2020-09-10T06:41:39Z

/run-all-tests

ZenoTan · 2020-09-10T07:13:48Z

/run-all-tests

ZenoTan · 2020-09-10T08:15:27Z

/run-test

ZenoTan · 2020-09-10T08:16:20Z

/run-integration-lightning-test

nolouch · 2020-09-10T08:22:37Z

/test

nolouch · 2020-09-10T08:26:26Z

/rebuild

nolouch · 2020-09-10T09:48:43Z

/merge

ti-srebot · 2020-09-10T09:49:17Z

/run-all-tests

ti-srebot added the contribution This PR is from a community contributor. label Jul 29, 2020

nolouch requested review from rleungx and nolouch July 29, 2020 14:32

rleungx reviewed Jul 31, 2020

View reviewed changes

nolouch reviewed Aug 3, 2020

View reviewed changes

nolouch reviewed Aug 4, 2020

View reviewed changes

rleungx reviewed Aug 25, 2020

View reviewed changes

ZenoTan closed this Sep 7, 2020

ZenoTan force-pushed the label branch from ea783c6 to a9cdb2d Compare September 7, 2020 03:24

Add basic change

4072fdc

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

ZenoTan reopened this Sep 7, 2020

nolouch reviewed Sep 8, 2020

View reviewed changes

server/cluster/cluster.go Outdated Show resolved Hide resolved

ZenoTan and others added 6 commits September 8, 2020 11:55

Change api

a473ec3

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

pd ctl support

a46f6b9

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

Merge branch 'master' into label

9664b3b

Fix

b5d19dd

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

Fix

79b144e

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

Merge branch 'label' of https://github.com/ZenoTan/pd into label

db87b59

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

nolouch reviewed Sep 8, 2020

View reviewed changes

nolouch reviewed Sep 9, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 9, 2020

rleungx reviewed Sep 10, 2020

View reviewed changes

ZenoTan and others added 3 commits September 10, 2020 10:34

Merge branch 'master' into label

13d99bf

Address comment

ef7cb44

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

Reformat

b35c553

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

ZenoTan added 2 commits September 10, 2020 10:46

Merge branch 'label' of https://github.com/ZenoTan/pd into label

c3704b3

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

Remove

291c46d

Signed-off-by: ZenoTan <zenotan1998@gmail.com>

rleungx approved these changes Sep 10, 2020

View reviewed changes

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 10, 2020

ti-srebot approved these changes Sep 10, 2020

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 10, 2020

ZenoTan added 2 commits September 10, 2020 11:17

Merge branch 'master' into label

92bc507

Merge branch 'master' into label

a421936

Merge branch 'master' into label

4c6c5f7

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 10, 2020

ti-srebot merged commit 62934dc into tikv:master Sep 10, 2020

rleungx changed the title ~~Change store limit by label~~ api: change store limit by label Sep 10, 2020

This was referenced Dec 28, 2021

api: support temporary configuration (#3082) #3088

Merged

Setting store limit by label is supported unexpectedly in v4.0 #4507

Closed

api: change store limit by label #2697

api: change store limit by label #2697

Conversation

ZenoTan commented Jul 29, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ZenoTan commented Jul 29, 2020

Choose a reason for hiding this comment

ZenoTan Jul 31, 2020 • 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

ZenoTan Aug 5, 2020 • 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

rleungx commented Aug 25, 2020

nolouch commented Sep 1, 2020 • edited Loading

ZenoTan commented Sep 3, 2020

ZenoTan commented Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

ZenoTan Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

nolouch Sep 9, 2020 • edited Loading

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

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

ZenoTan commented Sep 10, 2020

ti-srebot commented Sep 10, 2020

nolouch commented Sep 10, 2020

ti-srebot commented Sep 10, 2020

ti-srebot commented Sep 10, 2020

ZenoTan commented Sep 10, 2020

ZenoTan commented Sep 10, 2020

ZenoTan commented Sep 10, 2020

ZenoTan commented Sep 10, 2020

ZenoTan commented Sep 10, 2020

nolouch commented Sep 10, 2020

nolouch commented Sep 10, 2020

nolouch commented Sep 10, 2020

ti-srebot commented Sep 10, 2020

ZenoTan commented Jul 29, 2020 •

edited

Loading

ZenoTan Jul 31, 2020 •

edited

Loading

ZenoTan Aug 5, 2020 •

edited

Loading

nolouch commented Sep 1, 2020 •

edited

Loading

ZenoTan commented Sep 7, 2020 •

edited

Loading

ZenoTan Sep 9, 2020 •

edited

Loading

nolouch Sep 9, 2020 •

edited

Loading