server/schedule/labeler: support ttl for region label by AndreMouche · Pull Request #4695 · tikv/pd

AndreMouche · 2022-03-02T10:22:21Z

Signed-off-by: shirly AndreMouche@126.com

What problem does this PR solve?

Issue Number: Close #4694

What is changed and how it works?

support ttl for region label

usage , post a rule with ttl

#!/bin/bash

curl -X POST http://yourip:2379/pd/api/v1/config/region-label/rule -H "Content-Type: application/json" --data-binary @- <<DATA
{"id":"ttltest5",
 "labels": [{
   			"key": "schedule", 
                          "value": "deny",
			  "ttl":"2m" }], 
 "rule_type":"key-range",
 "data": [{
            "start_key": "7480000000000000ff455f720000000000fa",
            "end_key": "7480000000000000ff465f720000000000fa"
        }]
}
DATA

Check List

Tests

- Unit test
- Integration test

Release note

support `ttl` for region label.

Signed-off-by: shirly <AndreMouche@126.com>

ti-chi-bot · 2022-03-02T10:22:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

disksing
nolouch

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.

Details

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

AndreMouche · 2022-03-02T10:22:35Z

/wip

Signed-off-by: shirly <AndreMouche@126.com>

codecov · 2022-03-03T08:23:41Z

Codecov Report

Merging #4695 (e73ce60) into master (86e8d08) will increase coverage by 0.02%.
The diff coverage is 82.65%.

@@            Coverage Diff             @@
##           master    #4695      +/-   ##
==========================================
+ Coverage   75.21%   75.24%   +0.02%     
==========================================
  Files         294      294              
  Lines       28325    28416      +91     
==========================================
+ Hits        21306    21382      +76     
- Misses       5142     5155      +13     
- Partials     1877     1879       +2

Flag	Coverage Δ
unittests	`75.24% <82.65%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
server/cluster/cluster.go	`84.93% <ø> (-0.26%)`	⬇️
server/schedule/labeler/rules.go	`87.20% <77.50%> (-8.45%)`	⬇️
server/schedule/labeler/labeler.go	`76.77% <85.96%> (+3.69%)`	⬆️
pkg/mock/mockcluster/mockcluster.go	`94.98% <100.00%> (ø)`
pkg/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
server/schedulers/shuffle_hot_region.go	`55.55% <0.00%> (-10.11%)`	⬇️
server/schedulers/random_merge.go	`60.00% <0.00%> (-3.34%)`	⬇️
server/schedule/operator/step.go	`74.07% <0.00%> (-1.02%)`	⬇️
server/config/persist_options.go	`92.88% <0.00%> (-0.75%)`	⬇️
server/grpc_service.go	`52.04% <0.00%> (-0.65%)`	⬇️
... and 15 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 86e8d08...e73ce60. Read the comment docs.

Signed-off-by: shirly <AndreMouche@126.com>

server/schedule/labeler/labeler.go

server/cluster/cluster.go

server/schedule/labeler/labeler.go

Signed-off-by: shirly <AndreMouche@126.com>

nolouch · 2022-03-16T05:22:53Z

server/cluster/cluster.go


 var backgroundJobInterval = 10 * time.Second

+const regionLabelGCInterval = time.Hour


maybe a shorter time？There is earlistExpireTime, most of the time do not need to do any thing.

I think we needn't check the earlistExpireTime too frequently because lots of labels expired in 1 hour shouldn't be a normal phenomenon？

nolouch · 2022-03-16T06:21:43Z

server/schedule/labeler/labeler.go

+
+func (l *RegionLabeler) getAndCheckRule(id string, now time.Time) *LabelRule {
+	l.Lock()
+	defer l.Unlock()


can release the lock before save to storage?

sorry we can't , since the rule is a pointer get from labelRules, Without lock it could be updated in the other goroutine(s) which is not thread safe.

nolouch · 2022-03-16T06:25:42Z

server/schedule/labeler/labeler.go

-	l.RLock()
-	defer l.RUnlock()
-	return l.labelRules[id]
+	return l.getAndCheckRule(id, time.Now())


Can we reduce the passing of the now parameter, which currently seems unnecessary?

sorry, we can't. since we should check if there exist expired labels before return the current rule.

rleungx · 2022-03-16T06:51:37Z

server/schedule/labeler/labeler.go

+			continue
+		}
+		if len(rule.Labels) == 0 {
+			err = l.storage.DeleteRegionRule(key)


What will happen if we fail to delete in storage but successfully in memory?

It will just log the error info and the iteam wouldn't be deleted until the next restart. However, if there is something wrong with the storage last for long, the restart of the server would be long with too many expired items.

rleungx · 2022-03-16T06:53:22Z

server/schedule/labeler/labeler.go

+		return rule
+	}
+	if len(rule.Labels) == 0 {
+		l.storage.DeleteRule(id)


Here It will do nothing here and the item wouldn't be deleted in the storage until restart. However, if there is something wrong with the storage last for long, the restart of the server would be long with too many expired items.

rleungx · 2022-03-16T06:55:21Z

server/schedule/labeler/labeler_test.go

+
+	labeler.checkAndClearExpiredLabels()
+
+	labeler.RLock()


Why need a lock here?

Because there is a goroutine(The GC function) running which may change the labeler.labelRules concurrently

server/schedule/labeler/rule_test.go

server/schedule/labeler/rules.go

Signed-off-by: shirly <AndreMouche@126.com>

server/schedule/labeler/rules.go

server/schedule/labeler/labeler.go

Signed-off-by: shirly <AndreMouche@126.com>

AndreMouche · 2022-03-22T09:21:40Z

Manual test:

tidb+pd+tikv

set region-label with ttl by pd for table sbtest2

#!/bin/bash

curl -X POST http://yourip:2379/pd/api/v1/config/region-label/rule -H "Content-Type: application/json" --data-binary @- <<DATA

{"id":"schema/sbtest/sbtest2",
 "labels": [{
   			  "key": "schedule", 
               "value": "deny",
			   "ttl":"2h" 
			  },
            {
               "key":"maintainer",
               "value":"shirly"
            }
           ], 
 "rule_type":"key-range",
 "data": [{
            "start_key": "7480000000000000ff455f720000000000fa",
            "end_key": "7480000000000000ff465f720000000000fa"
        }]
}
DATA

List region-label in tidb-client

mysql> select * from information_schema.attributes\G;
*************************** 1. row ***************************
        ID: schema/sbtest/sbtest2
      TYPE: key-range
ATTRIBUTES: "schedule=deny","maintainer=shirly"
    RANGES: [7480000000000000ff455f720000000000fa, 7480000000000000ff465f720000000000fa]

server/schedule/labeler/labeler.go

server/schedule/labeler/rules.go

Signed-off-by: shirly <AndreMouche@126.com>

nolouch

LGTM

disksing · 2022-03-24T03:59:34Z

server/schedule/labeler/labeler.go

+		delete(l.labelRules, id)
+		return nil
+	}
+	l.storage.SaveRegionRule(id, rule)


why need save here?

Because there exist expired label which need to be cleared.

server/schedule/labeler/rules.go

disksing

LGTM

disksing · 2022-03-28T07:10:57Z

server/schedule/labeler/labeler.go

 		storage:    storage,
 		labelRules: make(map[string]*LabelRule),
+		ctx:        ctx,
+		minExpire:  nil,


Suggested change

minExpire: nil,

disksing · 2022-03-29T04:12:29Z

/merge

ti-chi-bot · 2022-03-29T04:12:29Z

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

/run-all-tests

Details

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-03-29T04:12:31Z

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

Details

Commit hash: e73ce60

server/schedule/labeler: support ttl for region label

319fad8

Signed-off-by: shirly <AndreMouche@126.com>

ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 2, 2022

ti-chi-bot requested review from nolouch and rleungx March 2, 2022 10:22

AndreMouche and others added 4 commits March 3, 2022 10:35

server/schedule/labeler: fix bug

c396051

Signed-off-by: shirly <AndreMouche@126.com>

Merge branch 'master' into labeler_ttl

91cbcfb

server/schedule/labeler: fix bug

1063e27

Signed-off-by: shirly <AndreMouche@126.com>

Merge branch 'labeler_ttl' of github.com:AndreMouche/pd into labeler_ttl

5e55ed3

AndreMouche added 2 commits March 3, 2022 17:45

server/schedule/labeler: update default earlist expire time

c301923

Signed-off-by: shirly <AndreMouche@126.com>

add test for labelrule

d2df0ea

Signed-off-by: shirly <AndreMouche@126.com>

lightmelodies mentioned this pull request Mar 4, 2022

Support to pause scheduling on specific regions #4620

Closed

AndreMouche added 4 commits March 4, 2022 14:02

add test for labelrule

39e7714

Signed-off-by: shirly <AndreMouche@126.com>

add test for labelrule

57b9e21

Signed-off-by: shirly <AndreMouche@126.com>

add test for labelrule

e124cb9

Signed-off-by: shirly <AndreMouche@126.com>

add test for labelrule

ee35568

Signed-off-by: shirly <AndreMouche@126.com>

nolouch requested review from disksing and removed request for nolouch March 8, 2022 03:47

rleungx reviewed Mar 10, 2022

View reviewed changes

server/schedule/labeler/labeler.go Outdated Show resolved Hide resolved

server/schedule/labeler/labeler.go Outdated Show resolved Hide resolved

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

server/schedule/labeler/labeler.go Outdated Show resolved Hide resolved

AndreMouche added 4 commits March 14, 2022 17:06

region-labels: move ttl to region-labels

ebe887c

Signed-off-by: shirly <AndreMouche@126.com>

Merge branch 'master' into labeler_ttl

37dd851

region-labels: address comments

e57e2ce

Signed-off-by: shirly <AndreMouche@126.com>

region-labels: address comments

c178c4a

Signed-off-by: shirly <AndreMouche@126.com>

nolouch reviewed Mar 16, 2022

View reviewed changes

rleungx reviewed Mar 16, 2022

View reviewed changes

region-labels: address comments

b5ba51c

Signed-off-by: shirly <AndreMouche@126.com>

rleungx reviewed Mar 17, 2022

View reviewed changes

server/schedule/labeler/rules.go Outdated Show resolved Hide resolved

server/schedule/labeler/labeler.go Outdated Show resolved Hide resolved

region-labels: address comments

a7814b0

Signed-off-by: shirly <AndreMouche@126.com>

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

merge master & fix conflicts

62306ba

Signed-off-by: shirly <AndreMouche@126.com>

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

disksing requested review from nolouch and rleungx March 22, 2022 08:07

nolouch reviewed Mar 22, 2022

View reviewed changes

server/schedule/labeler/labeler.go Outdated Show resolved Hide resolved

disksing reviewed Mar 22, 2022

View reviewed changes

server/schedule/labeler/labeler.go Outdated Show resolved Hide resolved

server/schedule/labeler/rules.go Outdated Show resolved Hide resolved

AndreMouche added 2 commits March 23, 2022 11:46

region-labels: address comments

1a5ab2d

Signed-off-by: shirly <AndreMouche@126.com>

Merge branch 'master' into labeler_ttl

c67af0e

nolouch approved these changes Mar 23, 2022

View reviewed changes

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

disksing reviewed Mar 24, 2022

View reviewed changes

server/schedule/labeler/rules.go Show resolved Hide resolved

disksing reviewed Mar 28, 2022

View reviewed changes

Merge branch 'master' into labeler_ttl

e73ce60

disksing approved these changes Mar 29, 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 Mar 29, 2022

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

ti-chi-bot merged commit 6e335aa into tikv:master Mar 29, 2022

sleepymole mentioned this pull request Jun 6, 2022

lightning: support disable scheduler by key range pingcap/tidb#34130

Merged

4 tasks

sleepymole mentioned this pull request Jul 22, 2022

lightning: support table-level local-backend pingcap/tidb#35148

Closed


		var backgroundJobInterval = 10 * time.Second

		const regionLabelGCInterval = time.Hour

Conversation

AndreMouche commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreMouche commented Mar 2, 2022

Uh oh!

codecov bot commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreMouche Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndreMouche commented Mar 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nolouch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

disksing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

disksing commented Mar 29, 2022

Uh oh!

ti-chi-bot commented Mar 29, 2022

Uh oh!

ti-chi-bot commented Mar 29, 2022

Uh oh!

Reviewers

Assignees

AndreMouche commented Mar 2, 2022 •

edited

Loading

ti-chi-bot commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 3, 2022 •

edited

Loading

AndreMouche Mar 17, 2022 •

edited

Loading