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.

Sign up for GitHub

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

Jump to bottom

schedulers: fix cache item in hot peer cache and add more test #2463

Merged
merged 5 commits into from
Jun 17, 2020
Merged

schedulers: fix cache item in hot peer cache and add more test #2463

merged 5 commits into from
Jun 17, 2020

Conversation

lhy1024
Copy link
Contributor

@lhy1024 lhy1024 commented May 27, 2020 •
edited
Loading

Signed-off-by: lhy1024 [email protected]

What problem does this PR solve?

Fix #2468

What is changed and how it works?

Prepare the cache item in advance

Check List

Tests

  • Unit test

Release note

  • No release note

@lhy1024 lhy1024 added the component/scheduler Scheduler logic. label May 27, 2020
@lhy1024 lhy1024 requested review from nolouch and rleungx May 27, 2020 06:24
@sre-bot
Copy link
Contributor

sre-bot commented May 27, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@lhy1024
Copy link
Contributor Author

lhy1024 commented May 27, 2020

/rebuild

@sre-bot
Copy link
Contributor

sre-bot commented May 27, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@lhy1024
Copy link
Contributor Author

lhy1024 commented May 27, 2020

/rebuild

@nolouch
Copy link
Contributor

nolouch commented May 28, 2020

Could you add a test?

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020 •
edited
Loading

Codecov Report

Merging #2463 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2463      +/-   ##
==========================================
+ Coverage   77.11%   77.29%   +0.18%     
==========================================
  Files         204      204              
  Lines       22026    22032       +6     
==========================================
+ Hits        16985    17030      +45     
+ Misses       3757     3733      -24     
+ Partials     1284     1269      -15
Impacted Files Coverage Δ
server/statistics/hot_peer_cache.go 92.61% <100.00%> (+0.26%) ⬆️
pkg/etcdutil/etcdutil.go 88.40% <0.00%> (ø)
server/cluster/cluster.go 80.36% <0.00%> (+0.42%) ⬆️
server/handler.go 51.61% <0.00%> (+0.43%) ⬆️
client/client.go 71.10% <0.00%> (+0.50%) ⬆️
server/tso/tso.go 83.21% <0.00%> (+0.72%) ⬆️
pkg/btree/btree.go 87.65% <0.00%> (+0.80%) ⬆️
server/core/storage.go 75.20% <0.00%> (+0.82%) ⬆️
server/server.go 77.70% <0.00%> (+1.22%) ⬆️
server/grpc_service.go 60.34% <0.00%> (+1.35%) ⬆️
... and 2 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 d9447e4...6bdc2a8. Read the comment docs.

@lhy1024
Copy link
Contributor Author

lhy1024 commented Jun 10, 2020

PTAL @rleungx @nolouch

rleungx
rleungx reviewed Jun 11, 2020
View reviewed changes
server/statistics/hot_peer_cache.go
for _, storeID := range storeIDs {
isExpired := f.isRegionExpired(region, storeID)
isExpired := f.isRegionExpired(region, storeID) // transfer leader or remove peer
Copy link
Member

Choose a reason for hiding this comment

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

I realized that it seems we can directly update the cache instead of returning the item which needs to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will open another PR to merge them.

Copy link
Contributor

Choose a reason for hiding this comment

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

why directly update the cache? we need to check it first for reducing heartbeat lock overload.

rleungx
rleungx reviewed Jun 11, 2020
View reviewed changes
server/statistics/hot_peer_cache_test.go
const interval = uint64(60)

func schedule(operator operator, region *core.RegionInfo, kind FlowKind) *core.RegionInfo {
func schedule(operator operator, region *core.RegionInfo, kind FlowKind) (srcStore uint64, _ *core.RegionInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func schedule(operator operator, region *core.RegionInfo, kind FlowKind) (srcStore uint64, _ *core.RegionInfo) {
func schedule(operator operator, region *core.RegionInfo, kind FlowKind) (uint64, *core.RegionInfo) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to indicate the meaning of this return var, I think it is better than putting it in a comment

Copy link
Member

Choose a reason for hiding this comment

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

I want to indicate the meaning of this return var, I think it is better than putting it in a comment

I agree

@lhy1024 lhy1024 changed the title schedulers: fix cache item in hot peer cache schedulers: fix cache item in hot peer cache and add more test Jun 12, 2020
@nolouch
Copy link
Contributor

nolouch commented Jun 15, 2020

cc @lhy1024

@lhy1024
Copy link
Contributor Author

lhy1024 commented Jun 15, 2020

PTAL @rleungx

@nolouch nolouch added needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. needs-cherry-pick-release-3.1 The PR needs to cherry pick to release-3.1 branch. labels Jun 17, 2020
@nolouch
Copy link
Contributor

nolouch commented Jun 17, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 17, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 136b778 into tikv:master Jun 17, 2020
@ti-srebot ti-srebot mentioned this pull request Jun 17, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 in PR #2548

@ti-srebot ti-srebot mentioned this pull request Jun 17, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #2549

ti-srebot added a commit that referenced this pull request Jun 18, 2020
@ti-srebot
schedulers: fix cache item in hot peer cache and add more test (#2463) (
b81f851 
#2549)

Signed-off-by: lhy1024 <[email protected]>
ti-srebot added a commit that referenced this pull request Jun 18, 2020
@ti-srebot
schedulers: fix cache item in hot peer cache and add more test (#2463) (
176f1e8 
#2548)

Signed-off-by: lhy1024 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleungx rleungx rleungx left review comments

@HunDunDM HunDunDM HunDunDM approved these changes

@nolouch nolouch nolouch approved these changes

Assignees
No one assigned
Labels
component/scheduler Scheduler logic. needs-cherry-pick-release-3.1 The PR needs to cherry pick to release-3.1 branch. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cache item maybe missed in hot peer cache
7 participants
@lhy1024 @sre-bot @nolouch @codecov-commenter @ti-srebot @HunDunDM @rleungx