*: refine code, decrease indent, and rename #8276

okJiang · 2024-06-11T06:43:18Z

What problem does this PR solve?

Issue Number: Ref #4399

What is changed and how does it work?

- decrease indent for code readability
- use `continue loop` to instead with `foundDisabled` variable, this is more concise
- no other logic updates
- rename `process` to `progress`

Check List

Tests

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

Release note

None.

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

codecov · 2024-06-11T06:56:31Z

Codecov Report

Attention: Patch coverage is 77.92208% with 17 lines in your changes missing coverage. Please review.

Project coverage is 77.32%. Comparing base (9dff6e6) to head (44c9cb0).

❗ Current head 44c9cb0 differs from pull request most recent head a8685db

Please upload reports for the commit a8685db to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8276      +/-   ##
==========================================
- Coverage   77.35%   77.32%   -0.03%     
==========================================
  Files         470      471       +1     
  Lines       61376    61397      +21     
==========================================
- Hits        47478    47476       -2     
- Misses      10336    10365      +29     
+ Partials     3562     3556       -6

Flag	Coverage Δ
unittests	`77.32% <77.92%> (-0.03%)`	⬇️

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

HuSharp · 2024-06-11T08:40:34Z

pkg/schedule/schedulers/scheduler_controller.go

-			return ops
+			if labelMgr.ScheduleDisabled(region) {
+				denySchedulersByLabelerCounter.Inc()
+				continue loop


Did we forget to update s.nextInterval = s.Scheduler.GetMinInterval() when executing ops[0] successfully?

maybe continue loop will not pass 494?

fixed in f1c1a1e

Seem the Reset does not work

pd/pkg/schedule/schedulers/scheduler_controller.go

Lines 360 to 361 in c015f14

// Note: we reset the ticker here to support updating configuration dynamically.

ticker.Reset(s.GetInterval())

Interval is fixed.

But it is not related to this pr

Is it necessary to make this change?

Did you mean the changes of this file or some others?

The changes of this file are:

decrease indent for code readability

use continue loop to instead with foundDisabled variable, this is more concise

no other updates

pkg/schedule/schedulers/scheduler_controller.go

HuSharp · 2024-06-11T09:17:37Z

server/cluster/cluster.go

+	storesSpeedGauge.WithLabelValues(storeAddress, storeLabel, action).Set(leftSeconds)
+	storesETAGauge.WithLabelValues(storeAddress, storeLabel, action).Set(currentSpeed)


Suggested change

storesSpeedGauge.WithLabelValues(storeAddress, storeLabel, action).Set(leftSeconds)

storesETAGauge.WithLabelValues(storeAddress, storeLabel, action).Set(currentSpeed)

storesSpeedGauge.WithLabelValues(storeAddress, storeLabel, action).Set(currentSpeed)

storesETAGauge.WithLabelValues(storeAddress, storeLabel, action).Set(leftSeconds)

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

rleungx · 2024-06-12T06:52:08Z

pkg/schedule/schedulers/scheduler_controller.go

-			// try regenerating operators
-			if foundDisabled {
+			region := s.cluster.GetRegion(op.RegionID())
+			if region == nil {
 				continue


I find that if the region == nil, do we still need to count it in ops?

pd/pkg/schedule/labeler/labeler.go

Line 324 in c015f14

if i, data := l.rangeList.GetData(region.GetStartKey(), region.GetEndKey()); i != -1 {

~~seems it will be panic~~

267dc63

region == nil means the ops are not normal right now, so can just retry

JmPotato · 2024-06-13T03:18:30Z

pkg/progress/progress.go

@@ -201,39 +204,40 @@ func (m *Manager) GetProgresses(filter func(p string) bool) []string {
 	m.RLock()
 	defer m.RUnlock()

-	processes := []string{}
+	progresses := []string{}


Suggested change

progresses := []string{}

progresses := make([]string, 0, len(m.progresses))

fixed in c107129

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

okJiang · 2024-06-14T06:11:25Z

Can you take a look again~ @rleungx @HuSharp @JmPotato

ti-chi-bot · 2024-06-14T06:24:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, 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~~ [JmPotato,rleungx]

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

ti-chi-bot · 2024-06-14T06:24:07Z

[LGTM Timeline notifier]

Timeline:

2024-06-14 06:14:55.417063803 +0000 UTC m=+704449.470375723: ☑️ agreed by rleungx.
2024-06-14 06:24:06.859579548 +0000 UTC m=+705000.912891469: ☑️ agreed by JmPotato.

JmPotato · 2024-06-14T06:24:41Z

/merge

ti-chi-bot · 2024-06-14T06:24:43Z

@JmPotato: We have migrated to builtin LGTM and approve plugins for reviewing.

👉 Please use /approve when you want approve this pull request.

The changes announcement: Proposal: Strengthen configuration change approval.

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.

okJiang added 5 commits May 28, 2024 14:48

save work

514c7b1

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

save work

fca37b4

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

ops is useless

6a2800c

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

decrease indent

eda3033

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

Merge branch 'master' of github.com:tikv/pd into refine

ad91aac

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

make check

9ab1abb

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

HuSharp reviewed Jun 11, 2024

View reviewed changes

okJiang added 2 commits June 11, 2024 17:21

fix comment

f1c1a1e

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

fix comment: typo

e9d975b

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

rleungx reviewed Jun 12, 2024

View reviewed changes

JmPotato reviewed Jun 13, 2024

View reviewed changes

okJiang added 3 commits June 13, 2024 11:51

fix comment

c107129

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

fix comment: retry when region is empty

267dc63

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

Merge branch 'master' of github.com:tikv/pd into refine

44c9cb0

rleungx approved these changes Jun 14, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 14, 2024

JmPotato approved these changes Jun 14, 2024

View reviewed changes

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

Merge branch 'master' into refine

a8685db

ti-chi-bot bot merged commit f1e85de into tikv:master Jun 14, 2024
16 checks passed

okJiang deleted the refine branch June 14, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: refine code, decrease indent, and rename #8276

*: refine code, decrease indent, and rename #8276

okJiang commented Jun 11, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

HuSharp Jun 11, 2024

okJiang Jun 11, 2024

HuSharp Jun 11, 2024 •

edited

Loading

okJiang Jun 11, 2024 •

edited

Loading

rleungx Jun 12, 2024

okJiang Jun 12, 2024

HuSharp Jun 11, 2024

rleungx Jun 12, 2024

okJiang Jun 12, 2024 •

edited

Loading

okJiang Jun 13, 2024

JmPotato Jun 13, 2024

okJiang Jun 13, 2024

okJiang commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

JmPotato commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

	// Note: we reset the ticker here to support updating configuration dynamically.
	ticker.Reset(s.GetInterval())

		storesSpeedGauge.WithLabelValues(storeAddress, storeLabel, action).Set(leftSeconds)
		storesETAGauge.WithLabelValues(storeAddress, storeLabel, action).Set(currentSpeed)

	progresses := []string{}
	progresses := make([]string, 0, len(m.progresses))

*: refine code, decrease indent, and rename #8276

*: refine code, decrease indent, and rename #8276

Conversation

okJiang commented Jun 11, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

codecov bot commented Jun 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSharp Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

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

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

okJiang commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

[LGTM Timeline notifier]

JmPotato commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

okJiang commented Jun 11, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

HuSharp Jun 11, 2024 •

edited

Loading

okJiang Jun 11, 2024 •

edited

Loading

okJiang Jun 12, 2024 •

edited

Loading