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

statistic: collect both read and write pending influence at the same time #5521

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

lhy1024
Copy link
Contributor

@lhy1024 lhy1024 commented Sep 16, 2022

Signed-off-by: lhy1024 [email protected]

What problem does this PR solve?

Issue Number: Ref #4949

Although there is a regionPendings in the hot scheduler, there is only read or write load during tryAddPendingInfluence.

Hot write scheduler and hot read scheduler may affect each other but cannot be perceived by each other.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

Release note

None.

@lhy1024 lhy1024 requested a review from HunDunDM September 16, 2022 07:32
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 16, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • HunDunDM
  • rleungx

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 ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Sep 16, 2022
@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Base: 75.60% // Head: 75.56% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (1a76711) compared to base (1e1732c).
Patch coverage: 86.95% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5521      +/-   ##
==========================================
- Coverage   75.60%   75.56%   -0.05%     
==========================================
  Files         325      325              
  Lines       32017    32055      +38     
==========================================
+ Hits        24208    24221      +13     
- Misses       5722     5737      +15     
- Partials     2087     2097      +10     
Flag Coverage Δ
unittests 75.56% <86.95%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
server/cluster/cluster.go 82.84% <0.00%> (-0.17%) ⬇️
server/statistics/hot_cache_task.go 83.63% <72.72%> (-2.73%) ⬇️
server/statistics/kind.go 43.42% <83.33%> (+3.42%) ⬆️
server/statistics/hot_cache.go 90.47% <91.66%> (+0.19%) ⬆️
pkg/mock/mockcluster/mockcluster.go 94.18% <100.00%> (+0.01%) ⬆️
server/schedulers/hot_region.go 85.82% <100.00%> (+0.08%) ⬆️
server/statistics/hot_peer_cache.go 96.89% <100.00%> (+0.02%) ⬆️
server/schedulers/shuffle_hot_region.go 55.55% <0.00%> (-10.11%) ⬇️
pkg/tempurl/tempurl.go 60.00% <0.00%> (-10.00%) ⬇️
...erver/config/service_middleware_persist_options.go 91.66% <0.00%> (-8.34%) ⬇️
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lhy1024 lhy1024 marked this pull request as ready for review September 16, 2022 09:22
@ti-chi-bot ti-chi-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 16, 2022
@lhy1024 lhy1024 added the component/statistics Statistics for scheduling. label Sep 16, 2022
@lhy1024
Copy link
Contributor Author

lhy1024 commented Sep 16, 2022

/run-build-arm64 comment=true

@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2022

case Read:
return Write
}
return Read
Copy link
Contributor

Choose a reason for hiding this comment

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

why default read?

Copy link
Member

Choose a reason for hiding this comment

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

== Case Write:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are only two kind in RWType

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can panic if not match

infl := statistics.Influence{Loads: make([]float64, statistics.RegionStatCount), Count: 1}
bs.rwTy.SetFullLoadRates(infl.Loads, peer.GetLoads())
inverse := bs.rwTy.Inverse()
another := bs.GetHotPeerStat(inverse, peer.RegionID, peer.StoreID)
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer summary all loads at a same time (including read and write) in prepareForBalance, then get it from the stLoadInfos. it is at a same "snapshot" rather than get it one by one.

Copy link
Member

Choose a reason for hiding this comment

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

Now we don't need to use read and write statistics together. I think we can do this when we unify read and write schedulers later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it will be placed into together when we unify read and write schedulers.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2022
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 19, 2022
Copy link
Member

@HunDunDM HunDunDM 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

if stat := peers.Get(region.GetID()); stat != nil {
return stat.(*HotPeerStat).HotDegree >= hotDegree
if stat := peers.Get(regionID); stat != nil {
return stat.(*HotPeerStat)
Copy link
Member

Choose a reason for hiding this comment

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

Are there times when there may be insufficient statistical confidence?

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 19, 2022
@HunDunDM
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

/run-all-tests

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
Copy link
Member

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

Commit hash: 1a76711

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 19, 2022
@ti-chi-bot ti-chi-bot merged commit ccb97f3 into tikv:master Sep 19, 2022
ti-chi-bot pushed a commit that referenced this pull request Oct 14, 2022
ref #5521, fix #5527

Signed-off-by: lhy1024 <[email protected]>

Co-authored-by: 混沌DM <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/statistics Statistics for scheduling. release-note-none Denotes a PR that doesn't merit a release note. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants