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

*: fix bug when unsigned pk meets feedback ranges #10307

Merged
merged 9 commits into from
Jun 4, 2019
Merged

*: fix bug when unsigned pk meets feedback ranges #10307

merged 9 commits into from
Jun 4, 2019

Conversation

winoros
Copy link
Member

@winoros winoros commented Apr 29, 2019

What problem does this PR solve?

fix #10242

What is changed and how it works?

Port some codes from #7921

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Related changes

  • Need to be included in the release note

@winoros winoros added the type/bugfix This PR fixes a bug. label Apr 29, 2019
@winoros winoros requested review from eurekaka, qw4990 and alivxxx April 29, 2019 10:47
winoros
winoros commented Apr 29, 2019
View reviewed changes
statistics/histogram.go Outdated Show resolved Hide resolved
@winoros winoros force-pushed the feedback-unsigned-ranges branch from ad8ec79 to 0da34a4 Compare April 29, 2019 11:48
@winoros winoros requested a review from alivxxx April 29, 2019 11:51
@shenli
Copy link
Member

shenli commented Apr 30, 2019

Could you add some test cases?

@winoros
Copy link
Member Author

winoros commented Apr 30, 2019 •
edited
Loading

@eurekaka
We need to move it because of the following reasons.

  1. In TableRangesToKVRanges, we encode the ranger.Range to kv.KeyRange. Since we store unsigned pk as int pk in physical kv layer in fact, unsigned ones are stored before signed ones in fact.
  2. But the SplitRange will put the unsigned ones after signed ones since it stores logical value.
  3. And if the reader doesn't set KeepOrder true. We do give TableRangesToKVRanges a full range contain both unsigned part and signed part. Then we got a wrong Range slice after Histogram.SplitRange.

So if we need to call Histogram.SplitRange, we should call it earlier to avoid this bug.

@winoros
Copy link
Member Author

winoros commented Apr 30, 2019 •
edited
Loading

When adding tests, finding that though the response can be sent correctly, but the feedback cannot be updated correctly. Need more changes to correct it.
Master has the same problem, it can be replayed easily if we change the column type from tinyint to bigint in unit test TestUnsignedFeedbackRanges.
/cc @lamxTyler

eurekaka
eurekaka reviewed Apr 30, 2019
View reviewed changes
executor/table_reader.go
@@ -96,6 +96,9 @@ func (e *TableReaderExecutor) Open(ctx context.Context) error {
}

e.resultHandler = &tableResultHandler{}
if e.feedback != nil && e.feedback.Hist() != nil {
e.ranges = e.feedback.Hist().SplitRange(e.ranges)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, please add the comment here to inform that SplitRange must be placed before splitRanges and explain the reasons.

@winoros winoros force-pushed the feedback-unsigned-ranges branch from 0da34a4 to 0e3f073 Compare May 23, 2019 12:45
@winoros winoros requested a review from eurekaka May 23, 2019 12:46
@winoros
Copy link
Member Author

winoros commented May 23, 2019

@lamxTyler @eurekaka Addressed, PTAL

@eurekaka
Copy link
Contributor

@winoros CI failed, PTAL

@winoros
Copy link
Member Author

winoros commented May 24, 2019

@eurekaka fixed

@winoros
Copy link
Member Author

winoros commented May 27, 2019

@lamxTyler Fixed

alivxxx
alivxxx reviewed May 27, 2019
View reviewed changes
statistics/feedback.go Outdated Show resolved Hide resolved
alivxxx
alivxxx reviewed May 27, 2019
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

zz-jason
zz-jason approved these changes May 28, 2019
View reviewed changes
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

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label May 28, 2019
eurekaka
eurekaka approved these changes May 28, 2019
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit 02526b6 into pingcap:release-2.1 Jun 4, 2019
@winoros winoros deleted the feedback-unsigned-ranges branch June 13, 2019 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alivxxx alivxxx alivxxx left review comments

@eurekaka eurekaka eurekaka approved these changes

@zz-jason zz-jason zz-jason approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/statistics status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@winoros @shenli @eurekaka @zz-jason @alivxxx @qw4990