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

statistics: multidimensional hot peer cache #2140

Merged
merged 9 commits into from
Feb 21, 2020

Conversation

Luffbee
Copy link
Contributor

@Luffbee Luffbee commented Feb 18, 2020

What problem does this PR solve?

First step of #2139 . The hot peer cache is not working with multidimensional load.

What is changed and how it works?

Modify the TopN to support multidimensional load.

Check List

Tests

  • Unit test

@Luffbee Luffbee added type/enhancement The issue or PR belongs to an enhancement. component/schedule Scheduling logic. labels Feb 18, 2020
@Luffbee Luffbee added this to the v4.0.0-beta.1 milestone Feb 18, 2020
@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

Merging #2140 into master will decrease coverage by <.01%.
The diff coverage is 76.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2140      +/-   ##
==========================================
- Coverage   75.95%   75.94%   -0.01%     
==========================================
  Files         195      195              
  Lines       20376    20376              
==========================================
- Hits        15477    15475       -2     
- Misses       3704     3709       +5     
+ Partials     1195     1192       -3
Impacted Files Coverage Δ
server/config/config.go 85.92% <ø> (ø) ⬆️
server/core/store.go 78.62% <100%> (ø) ⬆️
server/cluster/coordinator.go 78.02% <100%> (ø) ⬆️
tests/pdctl/helper.go 100% <100%> (ø) ⬆️
server/core/store_option.go 100% <100%> (ø) ⬆️
server/api/store.go 67.18% <100%> (ø) ⬆️
server/cluster/cluster.go 81.14% <40%> (ø) ⬆️
server/schedule/operator_controller.go 81.09% <61.9%> (-0.18%) ⬇️
server/schedulers/evict_leader.go 72.94% <91.66%> (ø) ⬆️
pkg/etcdutil/etcdutil.go 78.26% <0%> (-4.35%) ⬇️
... and 18 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 d6f225e...d935c88. Read the comment docs.

server/statistics/topn.go Outdated Show resolved Hide resolved
server/statistics/hot_peer_cache.go Show resolved Hide resolved
server/statistics/topn.go Show resolved Hide resolved
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.

rest LGTM.

server/statistics/hot_peer.go Outdated Show resolved Hide resolved
server/statistics/topn.go Show resolved Hide resolved
@Luffbee Luffbee modified the milestones: v4.0.0-beta.1, v4.0.0-rc Feb 20, 2020
Copy link
Contributor

@shafreeck shafreeck left a comment

Choose a reason for hiding this comment

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

LGTM

@Luffbee
Copy link
Contributor Author

Luffbee commented Feb 21, 2020

PTAL @rleungx

}
}

func putPerm(c *C, tn *TopN, K, Total int, f func(x int) float64, isUpdate bool) {
Copy link
Member

Choose a reason for hiding this comment

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

K, Total should start with the lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed.

// NOTE: panic if k <= 0 or n <= 0.
func NewTopN(k, n int, ttl time.Duration) *TopN {
if k <= 0 || n <= 0 {
panic("invalid arguments for NewTopN")
Copy link
Member

Choose a reason for hiding this comment

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

How about printing the value of n and k?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

_ = tn.ttlLst.Remove(id)
tn.maintain()
return item
return
Copy link
Member

Choose a reason for hiding this comment

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

It seems no need to return

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 think return the removed item is a better API. And it is used in the test.

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

sre-bot commented Feb 21, 2020

/run-all-tests

@sre-bot sre-bot merged commit f8db500 into tikv:master Feb 21, 2020
@Luffbee Luffbee deleted the multi-dim-hot-cache branch February 21, 2020 09:06
@nolouch nolouch added the needs-cherry-pick-release-3.1 The PR needs to cherry pick to release-3.1 branch. label Mar 16, 2020
@nolouch
Copy link
Contributor

nolouch commented Mar 16, 2020

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented Mar 16, 2020

cherry pick to release-3.1 in PR #2244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/schedule Scheduling logic. needs-cherry-pick-release-3.1 The PR needs to cherry pick to release-3.1 branch. 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