checker: unify checker name and checker type #8316

okJiang · 2024-06-21T06:50:03Z

What problem does this PR solve?

Issue Number: ~~Ref #5845~~ Ref #8379

What is changed and how does it work?

* Use underscore format to represent a checker
* use dash format to represent a checker in monitoring

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

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

pkg/schedule/config/name.go

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

rleungx · 2024-06-21T07:59:30Z

pkg/schedule/filter/counter.go

@@ -83,8 +83,8 @@ var scopes = [scopeLen]string{

 	"evict-leader-scheduler",
 	"region-scatter",
-	"replica-checker",
-	"rule-checker",
+	"replica_checker",


Can we unify the - and _?

Yes, scheduler will be unified in the next pr~

The monitor might also need to be changed.

Do you mean the checker's monitor?

I'm not sure about that, maybe scheduler or maybe checker

The scheduler name has not been changed.

The checker monitor is no need to change, after I checked. They are all in the form of xxx_checker.

Just a reminder

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

okJiang · 2024-06-24T03:31:06Z

/cc @lhy1024

rleungx · 2024-06-24T03:44:48Z

pkg/schedule/checker/rule_checker.go

@@ -404,7 +403,7 @@ func (c *RuleChecker) allowLeader(fit *placement.RegionFit, peer *metapb.Peer) b
 	if s == nil {
 		return false
 	}
-	stateFilter := &filter.StoreStateFilter{ActionScope: "rule-checker", TransferLeader: true}
+	stateFilter := &filter.StoreStateFilter{ActionScope: "rule_checker", TransferLeader: true}


Will it affect the monitor?

Scope is by group, so it will not affect the monitor. It will generate a new name after the new version goes live, but it won't have any actual impact.

Will it become xxx_checker-store which mixes hyphen and underscore?

Indeed. We want to use underscores in the code and hyphens for monitoring, right?

Waiting for confirmation~

IMO, it's better to use hyphens in the monitor.

rleungx · 2024-06-24T03:45:52Z

pkg/schedule/checker/rule_checker.go

@@ -108,7 +107,7 @@ func NewRuleChecker(ctx context.Context, cluster sche.CheckerCluster, ruleManage

 // GetType returns RuleChecker's Type
 func (*RuleChecker) GetType() string {
-	return ruleCheckerName
+	return config.RuleCheckerName.String()


After this PR, the type will be changed from xx-xx to xx_xx. It might affect the current monitor.

https://github.com/tikv/pd/pull/8316/files#r1650478721

rleungx · 2024-06-24T09:58:35Z

pkg/schedule/operator/metrics.go

+// IncOperatorLimitCounter increases the counter of operator meeting limit.
+func IncOperatorLimitCounter(typ, name string) {
+	operatorLimitCounter.WithLabelValues(
+		strings.ReplaceAll(typ, "_", "-"), name).Inc()


Is it possible not to replace it every time?

Is it for performance reasons? In that case, we still need to cache one more xxxx-xxxx for monitoring, which brings us back to the original issue.

How about using a map to do this?

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

okJiang · 2024-06-28T10:20:47Z

/hold Need to be discuss

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

ti-chi-bot · 2024-07-02T06:23:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign easonn7, husharp for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

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

rleungx · 2024-07-03T02:36:11Z

plugin/scheduler_example/evict_leader.go

-	EvictLeaderName        config.CheckerSchedulerName = "user-evict-leader-scheduler"
-	noStoreInSchedulerInfo                             = "No store in user-evict-leader-scheduler-config"
-)
+const noStoreInSchedulerInfo = "No store in user-evict-leader-scheduler-config"


no need to define a constant

rleungx · 2024-07-03T02:38:43Z

BTW, this PR is too large to review. Is it possible to split them?

okJiang · 2024-07-03T02:59:15Z

BTW, this PR is too large to review. Is it possible to split them?

After the ci pass, I will try it. But I still have to say, splitting is really tough😭 @rleungx

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

codecov · 2024-07-04T08:02:32Z

Codecov Report

Attention: Patch coverage is 75.76471% with 103 lines in your changes missing coverage. Please review.

Project coverage is 77.16%. Comparing base (86f8030) to head (823efe8).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8316      +/-   ##
==========================================
- Coverage   77.31%   77.16%   -0.15%     
==========================================
  Files         470      471       +1     
  Lines       61681    61688       +7     
==========================================
- Hits        47686    47600      -86     
- Misses      10405    10501      +96     
+ Partials     3590     3587       -3

Flag	Coverage Δ
unittests	`77.16% <75.76%> (-0.15%)`	⬇️

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

ti-chi-bot · 2024-07-09T08:43:00Z

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456, multiple issues should use full syntax for each issue and be separated by a comma, like: Issue Number: close #123, ref #456.

_{📖 For more info, you can check the "Linking issues" section in the CONTRIBUTING.md.}

ti-chi-bot · 2024-07-12T07:54:51Z

PR needs rebase.

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 kubernetes/test-infra repository.

unify checker name

1e2fcc8

Signed-off-by: okJiang <[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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2024

okJiang commented Jun 21, 2024

View reviewed changes

pkg/schedule/config/name.go Outdated Show resolved Hide resolved

fix lint

b0c59e1

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

okJiang changed the title ~~checker: unify checker name~~ checker: unify checker name and checker type Jun 21, 2024

Merge branch 'master' into unify-name

8d7dbac

rleungx reviewed Jun 21, 2024

View reviewed changes

okJiang added 2 commits June 21, 2024 17:25

update GetType

3d013e7

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

Merge branch 'unify-name' of github.com:okJiang/pd into unify-name

26e205a

ti-chi-bot bot requested a review from lhy1024 June 24, 2024 03:31

rleungx reviewed Jun 24, 2024

View reviewed changes

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2024

rleungx reviewed Jun 24, 2024

View reviewed changes

using map to convert checker type

9e87ceb

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

okJiang force-pushed the unify-name branch from a31dd0f to 9e87ceb Compare June 26, 2024 05:51

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 26, 2024

okJiang added 2 commits June 26, 2024 13:56

update

aadb26d

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

renaming xxxName to xxxType

58ab32b

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

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 Jun 28, 2024

okJiang added 3 commits July 1, 2024 15:50

Merge branch 'master' of github.com:tikv/pd into unify-name

4934f9d

save work

2f8dfe2

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

fix ut

7bed3bb

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

ti-chi-bot bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 2, 2024

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 2, 2024

okJiang added 4 commits July 2, 2024 14:29

unify function

7864c4c

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

revert one change

aa1b49b

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

modify alert and monitor

609e378

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

fix ci

5296ec1

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

rleungx reviewed Jul 3, 2024

View reviewed changes

okJiang marked this pull request as draft July 3, 2024 03:08

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

okJiang added 8 commits July 3, 2024 14:13

fix ut

a7266c2

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

update name

87dd198

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

rename

a6849ea

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

fix ut

de29655

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

Merge branch 'master' of github.com:tikv/pd into unify-name

7eda865

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

rename to type

fe30bfa

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

fix ut

252e2dc

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

fix ci, revert

823efe8

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

okJiang mentioned this pull request Jul 9, 2024

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

Closed

ti-chi-bot bot added the do-not-merge/needs-linked-issue label Jul 9, 2024

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

okJiang closed this Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker: unify checker name and checker type #8316

checker: unify checker name and checker type #8316

okJiang commented Jun 21, 2024 •

edited

Loading

rleungx Jun 21, 2024

okJiang Jun 21, 2024 •

edited

Loading

rleungx Jun 21, 2024

okJiang Jun 21, 2024

rleungx Jun 21, 2024

okJiang Jun 21, 2024

rleungx Jun 21, 2024

okJiang commented Jun 24, 2024

rleungx Jun 24, 2024

okJiang Jun 24, 2024

rleungx Jun 24, 2024 •

edited

Loading

okJiang Jun 24, 2024

rleungx Jun 24, 2024 •

edited

Loading

rleungx Jun 24, 2024 •

edited

Loading

okJiang Jun 24, 2024

rleungx Jun 24, 2024

okJiang Jun 24, 2024 •

edited

Loading

okJiang Jun 25, 2024

okJiang Jun 26, 2024 •

edited

Loading

okJiang commented Jun 28, 2024

ti-chi-bot bot commented Jul 2, 2024

rleungx Jul 3, 2024

rleungx commented Jul 3, 2024

okJiang commented Jul 3, 2024

codecov bot commented Jul 4, 2024 •

edited

Loading

ti-chi-bot bot commented Jul 9, 2024

ti-chi-bot bot commented Jul 12, 2024

checker: unify checker name and checker type #8316

checker: unify checker name and checker type #8316

Conversation

okJiang commented Jun 21, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Choose a reason for hiding this comment

okJiang Jun 21, 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

Choose a reason for hiding this comment

okJiang commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

rleungx Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

okJiang commented Jun 28, 2024

ti-chi-bot bot commented Jul 2, 2024

Choose a reason for hiding this comment

rleungx commented Jul 3, 2024

okJiang commented Jul 3, 2024

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Jul 9, 2024

ti-chi-bot bot commented Jul 12, 2024

okJiang commented Jun 21, 2024 •

edited

Loading

okJiang Jun 21, 2024 •

edited

Loading

rleungx Jun 24, 2024 •

edited

Loading

rleungx Jun 24, 2024 •

edited

Loading

rleungx Jun 24, 2024 •

edited

Loading

okJiang Jun 24, 2024 •

edited

Loading

okJiang Jun 26, 2024 •

edited

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading