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

stats: merge non-overlapped feedback when update bucket count #10476

Merged
merged 6 commits into from
May 22, 2019

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented May 15, 2019

What problem does this PR solve?

Fix #10365

What is changed and how it works?

When updating the bucket count, we first try to merge the max fraction of non-overlapped feedback that is contained in the bucket, the result bucket count will be more accurate than only use one feedback.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

@alivxxx alivxxx added type/enhancement The issue or PR belongs to an enhancement. component/statistics labels May 15, 2019
@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #10476 into master will decrease coverage by 0.0111%.
The diff coverage is 77.2727%.

@@              Coverage Diff               @@
##             master   #10476        +/-   ##
==============================================
- Coverage   77.3311%   77.32%   -0.0112%     
==============================================
  Files           413      413                
  Lines         87274    87262        -12     
==============================================
- Hits          67490    67471        -19     
+ Misses        14602    14601         -1     
- Partials       5182     5190         +8

statistics/feedback.go Outdated Show resolved Hide resolved
statistics/feedback.go Show resolved Hide resolved
@alivxxx alivxxx requested review from XuHuaiyu and winoros and removed request for XuHuaiyu May 21, 2019 06:21
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm, can you also change the test mentioned in the issue?

@alivxxx alivxxx requested a review from XuHuaiyu May 22, 2019 03:15
@alivxxx
Copy link
Contributor Author

alivxxx commented May 22, 2019

@winoros Added.

@mahjonp
Copy link
Contributor

mahjonp commented May 22, 2019

/rebuild

statistics/feedback.go Outdated Show resolved Hide resolved
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx
Copy link
Contributor Author

alivxxx commented May 22, 2019

/run-all-tests

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed labels May 22, 2019
@alivxxx alivxxx merged commit 380eb2c into pingcap:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/statistics status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong feedback for unsigned column.
5 participants