coordinator: use a healthy region count to start coordinator #7044

HuSharp · 2023-09-05T03:38:52Z

What problem does this PR solve?

Issue Number: Close #6988 ,Close #7016

What is changed and how does it work?

different source of region, regard FromSync and FromStorage as the stale region
add a stale region count in regionTree to start coordinator first.
atomic add/sub stale regions number by region updated

Check List

Tests

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

Release note

use a healthy region count to avoid starting coordinator timeout.

ti-chi-bot · 2023-09-05T03:38:54Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bufferflies
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.

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

ti-chi-bot · 2023-09-05T03:38:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: husharp <jinhao.hu@pingcap.com>

rleungx · 2023-09-06T06:36:09Z

pkg/core/region.go

+
+const (
+	// InDisk means region is stale.
+	InDisk RegionSource = iota


How about FromLoad or FromStorage?

update to FromStorage

Signed-off-by: husharp <jinhao.hu@pingcap.com>

codecov · 2023-09-06T07:19:10Z

Codecov Report

Merging #7044 (a9d6e3e) into master (0adb86f) will increase coverage by 0.07%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #7044      +/-   ##
==========================================
+ Coverage   74.62%   74.69%   +0.07%     
==========================================
  Files         442      442              
  Lines       47660    47681      +21     
==========================================
+ Hits        35565    35615      +50     
+ Misses       8972     8947      -25     
+ Partials     3123     3119       -4

Flag	Coverage Δ
unittests	`74.69% <94.44%> (+0.07%)`	⬆️

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

pkg/core/region.go

Signed-off-by: husharp <jinhao.hu@pingcap.com>

Signed-off-by: husharp <ihusharp@gmail.com>

pkg/core/region_tree.go

JmPotato · 2023-09-11T05:45:21Z

pkg/core/region_tree.go

@@ -342,3 +346,20 @@ func (t *regionTree) TotalWriteRate() (bytesRate, keysRate float64) {
 	}
 	return t.totalWriteBytesRate, t.totalWriteKeysRate
 }
+
+func (t *regionTree) AtomicAddStaleRegionCnt() {
+	if t.length() == 0 {


Why do we need to check the length here?

Now we do not need to check it after moving staleRegionCnt into regionsInfo

pkg/schedule/prepare_checker.go

pkg/syncer/client.go

rleungx · 2023-09-11T07:08:15Z

pkg/schedule/prepare_checker.go

+		log.Info("stale meta region number is satisfied, skip prepare checker", zap.Int64("stale-region", c.GetStaleRegionCnt()), zap.Int("total-region", c.GetTotalRegionCount()))
+		checker.prepared = true
+		return true
+	}
 	// The number of active regions should be more than total region of all stores * collectFactor
 	if float64(c.GetTotalRegionCount())*collectFactor > float64(checker.sum) {


Do we still need it?

maybe it is better to remove them in another pr? This pr just focuses on accelerating coordinator.
What do u think :)

rleungx · 2023-09-11T07:11:20Z

pkg/core/region.go

+	// FromStorage means this region's meta info might be stale.
+	r.AtomicAddStaleRegionCnt()


Here we need to make sure that only reload from storage will use CheckAndPutRegion.

fixed by return regions num

rleungx · 2023-10-10T03:10:11Z

pkg/core/region.go

+)
+
+// IsSourceStale means this region's meta info might be stale.
+func (r *RegionInfo) IsSourceStale() bool {


How about not using stale/fresh? Just use LoadedFromStorage and !LoadedFromStorage. Or something like that.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch

lgtm

nolouch · 2023-10-13T03:54:53Z

/merge

ti-chi-bot · 2023-10-13T03:54:55Z

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

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 · 2023-10-13T03:54:57Z

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

Commit hash: 6281684

ti-chi-bot · 2023-10-13T03:55:10Z

@HuSharp: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

close tikv#6988, close tikv#7016 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot · 2023-11-14T05:31:03Z

In response to a cherrypick label: new pull request created to branch release-6.5: #7363.

close tikv#6988, close tikv#7016 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-11-14T05:33:46Z

In response to a cherrypick label: new pull request created to branch release-7.1: #7364.

close tikv#6988, close tikv#7016 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

…7363) close #6988, close #7016 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

…7364) close #6988, close #7016 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com>

close tikv#6988, close tikv#7016 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-triage-completed labels Sep 5, 2023

ti-chi-bot bot requested review from JmPotato and rleungx September 5, 2023 03:39

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2023

HuSharp force-pushed the fix_coordinator branch from c5d8bcf to 31e8d61 Compare September 6, 2023 02:53

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2023

HuSharp force-pushed the fix_coordinator branch from 31e8d61 to eb8db24 Compare September 6, 2023 02:54

HuSharp changed the title ~~coordinator: use a region count to start coordinator~~ coordinator: use a stale region count to start coordinator Sep 6, 2023

HuSharp force-pushed the fix_coordinator branch 2 times, most recently from d34b261 to 4b862b9 Compare September 6, 2023 03:42

fix

accb06b

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the fix_coordinator branch from 4b862b9 to accb06b Compare September 6, 2023 04:31

HuSharp marked this pull request as ready for review September 6, 2023 04:49

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2023

Merge branch 'master' into fix_coordinator

46406e7

rleungx reviewed Sep 6, 2023

View reviewed changes

address comment

d161acb

Signed-off-by: husharp <jinhao.hu@pingcap.com>

JmPotato reviewed Sep 6, 2023

View reviewed changes

pkg/core/region.go Outdated Show resolved Hide resolved

pkg/core/region.go Outdated Show resolved Hide resolved

pkg/core/region.go Outdated Show resolved Hide resolved

address comment

6be6f17

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the fix_coordinator branch from 18353dc to 6be6f17 Compare September 7, 2023 08:09

HuSharp added 2 commits September 11, 2023 10:44

Merge remote-tracking branch 'upstream/master' into fix_coordinator

1f16cda

address comment

68a1107

Signed-off-by: husharp <ihusharp@gmail.com>

JmPotato reviewed Sep 11, 2023

View reviewed changes

rleungx reviewed Sep 11, 2023

View reviewed changes

Merge branch 'master' into fix_coordinator

45d2cc5

rleungx reviewed Oct 10, 2023

View reviewed changes

address comment

6281684

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the fix_coordinator branch from 9574d70 to 6281684 Compare October 10, 2023 09:04

HuSharp removed needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. labels Oct 11, 2023

nolouch approved these changes Oct 13, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 13, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 13, 2023

Merge branch 'master' into fix_coordinator

a9d6e3e

ti-chi-bot bot merged commit 2ba7ed4 into tikv:master Oct 13, 2023

HuSharp deleted the fix_coordinator branch October 16, 2023 08:19

lhy1024 mentioned this pull request Oct 27, 2023

Datarace in TestPrepareChecker #7273

Closed

HuSharp added the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Nov 14, 2023

ti-chi-bot mentioned this pull request Nov 14, 2023

coordinator: use a healthy region count to start coordinator (#7044) #7363

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Nov 14, 2023

This is an automated cherry-pick of tikv#7044

b9c8941

close tikv#6988, close tikv#7016 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

HuSharp added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Nov 14, 2023

ti-chi-bot mentioned this pull request Nov 14, 2023

coordinator: use a healthy region count to start coordinator (#7044) #7364

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Nov 14, 2023

This is an automated cherry-pick of tikv#7044

081af40

close tikv#6988, close tikv#7016 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coordinator: use a healthy region count to start coordinator #7044

coordinator: use a healthy region count to start coordinator #7044

HuSharp commented Sep 5, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 5, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 5, 2023

rleungx Sep 6, 2023 •

edited

Loading

HuSharp Sep 6, 2023

codecov bot commented Sep 6, 2023 •

edited

Loading

JmPotato Sep 11, 2023

HuSharp Sep 13, 2023

rleungx Sep 11, 2023

HuSharp Sep 13, 2023 •

edited

Loading

rleungx Sep 11, 2023

HuSharp Sep 13, 2023

rleungx Oct 10, 2023

HuSharp Oct 10, 2023

nolouch left a comment

nolouch commented Oct 13, 2023

ti-chi-bot bot commented Oct 13, 2023

ti-chi-bot bot commented Oct 13, 2023

ti-chi-bot bot commented Oct 13, 2023

ti-chi-bot commented Nov 14, 2023

ti-chi-bot commented Nov 14, 2023

		// FromStorage means this region's meta info might be stale.
		r.AtomicAddStaleRegionCnt()

coordinator: use a healthy region count to start coordinator #7044

coordinator: use a healthy region count to start coordinator #7044

Conversation

HuSharp commented Sep 5, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Sep 5, 2023 • edited Loading

ti-chi-bot bot commented Sep 5, 2023

rleungx Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSharp Sep 13, 2023 • 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

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Oct 13, 2023

ti-chi-bot bot commented Oct 13, 2023

ti-chi-bot bot commented Oct 13, 2023

ti-chi-bot bot commented Oct 13, 2023

ti-chi-bot commented Nov 14, 2023

ti-chi-bot commented Nov 14, 2023

HuSharp commented Sep 5, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 5, 2023 •

edited

Loading

rleungx Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

HuSharp Sep 13, 2023 •

edited

Loading