Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: support range for random selection #1783

Merged
merged 10 commits into from
Oct 21, 2019
Merged

core: support range for random selection #1783

merged 10 commits into from
Oct 21, 2019

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Sep 29, 2019

What problem does this PR solve?

This PR is mainly for supporting range for schedulers.

What is changed and how it works?

This PR changes the region map to region tree and supports random selection according to a given range.

Check List

Tests

  • Unit test

Side effects

  • Possible performance regression

@rleungx rleungx added the component/schedule Scheduling logic. label Sep 29, 2019
@nolouch nolouch added the type/enhancement The issue or PR belongs to an enhancement. label Sep 29, 2019
@codecov-io
Copy link

codecov-io commented Sep 30, 2019

Codecov Report

Merging #1783 into master will decrease coverage by 0.15%.
The diff coverage is 87.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1783      +/-   ##
==========================================
- Coverage    78.1%   77.95%   -0.16%     
==========================================
  Files         167      167              
  Lines       16851    16907      +56     
==========================================
+ Hits        13162    13180      +18     
- Misses       2613     2642      +29     
- Partials     1076     1085       +9
Impacted Files Coverage Δ
server/core/basic_cluster.go 95% <100%> (ø) ⬆️
server/core/storage.go 76.31% <100%> (ø) ⬆️
server/cluster.go 84.91% <50%> (ø) ⬆️
server/core/region_storage.go 81.81% <50%> (ø) ⬆️
server/core/region_tree.go 93.57% <83.72%> (-4.18%) ⬇️
server/core/region.go 91.73% <90.1%> (-0.96%) ⬇️
server/core/test_util.go 96.2% <92.85%> (+0.09%) ⬆️
pkg/metricutil/metricutil.go 90.62% <0%> (-9.38%) ⬇️
server/kv/etcd_kv.go 72.72% <0%> (-9.1%) ⬇️
server/schedulers/shuffle_hot_region.go 56.17% <0%> (-5.62%) ⬇️
... and 7 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 c456deb...993cd76. Read the comment docs.

@rleungx
Copy link
Member Author

rleungx commented Sep 30, 2019

master:
BenchmarkRandomRegion-40    	50000000	        37.0 ns/op	       0 B/op	       0 allocs/op
PASS

tree:
BenchmarkRandomRegion-40    	 1000000	      1688 ns/op	       0 B/op	       0 allocs/op
PASS

@rleungx rleungx force-pushed the tree branch 4 times, most recently from 757e9eb to dd25be6 Compare October 9, 2019 05:44
@rleungx
Copy link
Member Author

rleungx commented Oct 9, 2019

master:
BenchmarkAddRegion-40    	  500000	      4909 ns/op	    1120 B/op	      21 allocs/op

tree:
BenchmarkAddRegion-40    	  300000	      5662 ns/op	    1797 B/op	      45 allocs/op

server/core/region.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region.go Show resolved Hide resolved
@@ -460,25 +452,95 @@ func (rm *regionMap) TotalSize() int64 {
return rm.totalSize
}

// regionSubTree is used to manager different types of regions.
type regionSubTree struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name regionSubTree and regionTree seems confused. Maybe we should name it according to the purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I cannot find a proper name.

server/core/region.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree_test.go Show resolved Hide resolved
server/core/region_tree.go Show resolved Hide resolved
server/core/region.go Outdated Show resolved Hide resolved
@nolouch nolouch added the priority/P1 The issue has P1 priority. label Oct 15, 2019
@rleungx rleungx requested a review from Luffbee October 16, 2019 05:41
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. PTAL @Luffbee

server/core/region.go Outdated Show resolved Hide resolved
server/core/region_tree_test.go Outdated Show resolved Hide resolved
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Copy link
Contributor

@Luffbee Luffbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nolouch nolouch added the status/can-merge Indicates a PR has been approved by a committer. label Oct 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

/run-all-tests

@sre-bot sre-bot merged commit 752ecd7 into tikv:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/schedule Scheduling logic. priority/P1 The issue has P1 priority. status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants