scheduler: add more hot scheduler comments and replace negative rank #8345

lhy1024 · 2024-06-27T15:11:03Z

What problem does this PR solve?

Issue Number: Ref #5691

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None.

Signed-off-by: lhy1024 <admin@liudos.us>

codecov · 2024-07-03T03:33:25Z

Codecov Report

Attention: Patch coverage is 94.91525% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.36%. Comparing base (2baee83) to head (635d720).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8345      +/-   ##
==========================================
+ Coverage   77.33%   77.36%   +0.02%     
==========================================
  Files         472      472              
  Lines       61802    61799       -3     
==========================================
+ Hits        47793    47808      +15     
+ Misses      10430    10419      -11     
+ Partials     3579     3572       -7

Flag	Coverage Δ
unittests	`77.36% <94.91%> (+0.02%)`	⬆️

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

HuSharp · 2024-07-03T03:55:54Z

/test pull-integration-realcluster-test

pkg/schedule/schedulers/hot_region.go

okJiang · 2024-07-10T06:15:00Z

pkg/schedule/schedulers/hot_region.go

+// | ↓ firstPriority \ secondPriority → | isBetter | isNotWorsened | Worsened  |
+// |   isBetter                         | 4        | 3             | 1         |
+// |   isNotWorsened                    | 2        | -1            | -1        |
+// |   Worsened                         | 0        | -1            | -1        |
 func (bs *balanceSolver) calcProgressiveRankV1() {


How about using variables to represent instead of magic numbers?

It is the score to be used for comparing.

Yes, it is. But it is still being used in many places for making judgments, which indeed can be hard to understand in the code, so more comments have to be added to explain it. Like these

pd/pkg/schedule/schedulers/hot_region.go

Lines 1373 to 1393 in cbba49a

switch bs.cur.progressiveRank {

case -4: // isBetter(firstPriority) && isBetter(secondPriority)

if firstCmp != 0 {

return firstCmp > 0

}

return secondCmp > 0

case -3: // isBetter(firstPriority) && isNotWorsened(secondPriority)

if firstCmp != 0 {

return firstCmp > 0

}

// prefer smaller second priority rate, to reduce oscillation

return secondCmp < 0

case -2: // isNotWorsened(firstPriority) && isBetter(secondPriority)

if secondCmp != 0 {

return secondCmp > 0

}

// prefer smaller first priority rate, to reduce oscillation

return firstCmp < 0

case -1: // isBetter(firstPriority)

return firstCmp > 0

// TODO: The smaller the difference between the value and the expectation, the better.

I feel like it's really hard to name, so I'm just suggesting something and not rushing to solve it. :)

Signed-off-by: lhy1024 <admin@liudos.us>

pkg/schedule/schedulers/hot_region_rank_v1.go

rleungx · 2024-07-25T08:55:09Z

pkg/statistics/store_hot_peers_infos.go

-			// Metric for debug.
-			// TODO: pre-allocate gauge metrics
-			ty := "byte-rate-" + rwTy.String() + "-" + kind.String()
-			hotPeerSummary.WithLabelValues(ty, fmt.Sprintf("%v", id)).Set(peerLoadSum[utils.ByteDim])


no need any more?

Yeah, we haven't been using this metrics

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2024-07-25T09:18:56Z

@okJiang: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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.

pkg/schedule/schedulers/hot_region_rank_v1.go

Signed-off-by: lhy1024 <admin@liudos.us>

pkg/schedule/schedulers/hot_region.go

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2024-07-25T15:55:26Z

[LGTM Timeline notifier]

Timeline:

2024-07-25 09:04:32.030124632 +0000 UTC m=+1122294.021066100: ☑️ agreed by rleungx.
2024-07-25 15:55:25.056457244 +0000 UTC m=+1146947.047398699: ☑️ agreed by HuSharp.

lhy1024 · 2024-07-26T02:17:11Z

pkg/schedule/schedulers/hot_region_config.go

@@ -38,7 +38,6 @@ import (
 )

 const (
-


Because modify config file, PTAL cc @niubell

lhy1024 · 2024-07-26T06:32:20Z

/test pull-integration-realcluster-test

ti-chi-bot · 2024-07-26T06:40:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuSharp, niubell, okJiang, 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~~ [HuSharp,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

lhy1024 added 2 commits June 27, 2024 23:08

scheduler: add more comments

29ee921

Signed-off-by: lhy1024 <admin@liudos.us>

replace negetive rank

6a9c0af

Signed-off-by: lhy1024 <admin@liudos.us>

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 27, 2024

lhy1024 and others added 2 commits July 1, 2024 11:57

fix lint

0fc3fd9

Signed-off-by: lhy1024 <admin@liudos.us>

Merge branch 'master' into hot-comments

2df33eb

ti-chi-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 5, 2024

okJiang reviewed Jul 10, 2024

View reviewed changes

lhy1024 added 2 commits July 15, 2024 18:07

Merge branch 'master' of github.com:tikv/pd into hot-comments

025bac3

Signed-off-by: lhy1024 <admin@liudos.us>

address comments

13aba13

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the hot-comments branch from 7ffe7c9 to 13aba13 Compare July 15, 2024 10:08

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

Merge branch 'master' of github.com:tikv/pd into hot-comments

ea0ca3e

Signed-off-by: lhy1024 <admin@liudos.us>

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

fix comments

9fa4bfd

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Jul 25, 2024

View reviewed changes

address comments

efb94dd

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx approved these changes Jul 25, 2024

View reviewed changes

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

okJiang approved these changes Jul 25, 2024

View reviewed changes

HuSharp reviewed Jul 25, 2024

View reviewed changes

pkg/schedule/schedulers/hot_region_rank_v1.go Show resolved Hide resolved

address comments

9391e20

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp reviewed Jul 25, 2024

View reviewed changes

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

address comments

1f002c1

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp approved these changes Jul 25, 2024

View reviewed changes

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

lhy1024 commented Jul 26, 2024

View reviewed changes

Merge branch 'master' into hot-comments

635d720

niubell approved these changes Jul 26, 2024

View reviewed changes

ti-chi-bot bot added the approved label Jul 26, 2024

ti-chi-bot bot merged commit 024656b into tikv:master Jul 26, 2024
25 checks passed

lhy1024 deleted the hot-comments branch July 26, 2024 06:44

okJiang mentioned this pull request Apr 27, 2025

HistoryLoads check are always skipped during hot_region_scheduler #9270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

scheduler: add more hot scheduler comments and replace negative rank #8345

scheduler: add more hot scheduler comments and replace negative rank #8345

Uh oh!

lhy1024 commented Jun 27, 2024

Uh oh!

codecov bot commented Jul 3, 2024 •

edited

Loading

Uh oh!

HuSharp commented Jul 3, 2024

Uh oh!

Uh oh!

Uh oh!

okJiang Jul 10, 2024

Uh oh!

lhy1024 Jul 15, 2024 •

edited

Loading

Uh oh!

okJiang Jul 17, 2024

Uh oh!

Uh oh!

rleungx Jul 25, 2024

Uh oh!

lhy1024 Jul 25, 2024

Uh oh!

ti-chi-bot bot commented Jul 25, 2024

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot bot commented Jul 25, 2024

Uh oh!

lhy1024 Jul 26, 2024

Uh oh!

lhy1024 commented Jul 26, 2024

Uh oh!

ti-chi-bot bot commented Jul 26, 2024

Uh oh!

Uh oh!

Uh oh!

	switch bs.cur.progressiveRank {
	case -4: // isBetter(firstPriority) && isBetter(secondPriority)
	if firstCmp != 0 {
	return firstCmp > 0
	}
	return secondCmp > 0
	case -3: // isBetter(firstPriority) && isNotWorsened(secondPriority)
	if firstCmp != 0 {
	return firstCmp > 0
	}
	// prefer smaller second priority rate, to reduce oscillation
	return secondCmp < 0
	case -2: // isNotWorsened(firstPriority) && isBetter(secondPriority)
	if secondCmp != 0 {
	return secondCmp > 0
	}
	// prefer smaller first priority rate, to reduce oscillation
	return firstCmp < 0
	case -1: // isBetter(firstPriority)
	return firstCmp > 0
	// TODO: The smaller the difference between the value and the expectation, the better.

scheduler: add more hot scheduler comments and replace negative rank #8345

scheduler: add more hot scheduler comments and replace negative rank #8345

Uh oh!

Conversation

lhy1024 commented Jun 27, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

HuSharp commented Jul 3, 2024

Uh oh!

Uh oh!

Uh oh!

okJiang Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

lhy1024 Jul 15, 2024 • 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!

okJiang Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rleungx Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

lhy1024 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jul 25, 2024

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot bot commented Jul 25, 2024

[LGTM Timeline notifier]

Uh oh!

lhy1024 Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

lhy1024 commented Jul 26, 2024

Uh oh!

ti-chi-bot bot commented Jul 26, 2024

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 3, 2024 •

edited

Loading

lhy1024 Jul 15, 2024 •

edited

Loading