-
Notifications
You must be signed in to change notification settings - Fork 720
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: batch get region size #7252
Conversation
Signed-off-by: Ryan Leung <rleungx@gmail.com>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Codecov Report
@@ Coverage Diff @@
## master #7252 +/- ##
==========================================
- Coverage 74.41% 74.41% -0.01%
==========================================
Files 446 446
Lines 48423 48433 +10
==========================================
+ Hits 36034 36040 +6
+ Misses 9203 9201 -2
- Partials 3186 3192 +6
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix 7248?
pkg/core/region.go
Outdated
func (r *RegionsInfo) GetRegionSizeByRange(startKey, endKey []byte) int64 { | ||
r.t.RLock() | ||
defer r.t.RUnlock() | ||
func (r *RegionsInfo) GetRegionSizeByRange(startKey, endKey []byte, limit int) int64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add bench for it?
can we do some simulate test? |
Signed-off-by: nolouch <nolouch@gmail.com>
I added a benchmark and ran it in my local MBP, it shows some improvement. master
this branch:
|
d25c907
to
7982009
Compare
7982009
to
2a8ae9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but ci failed
c6f20a1
to
1dbb94f
Compare
/merge |
@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
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. |
This pull request has been accepted and is ready to merge. Commit hash: 56aac2d
|
In response to a cherrypick label: new pull request created to branch |
close tikv#7248 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
In response to a cherrypick label: new pull request created to branch |
close tikv#7248 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
In response to a cherrypick label: new pull request created to branch |
/run-cherry-picker |
In response to a cherrypick label: new pull request created to branch |
close tikv#7248 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
What problem does this PR solve?
Issue Number: Close #7248.
What is changed and how does it work?
Check List
Tests
Release note