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

*: merge statement buffer when BatchGetValues #9374

Merged
merged 6 commits into from
Feb 22, 2019
Merged

*: merge statement buffer when BatchGetValues #9374

merged 6 commits into from
Feb 22, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Feb 20, 2019 •
edited
Loading

What problem does this PR solve?

The BatchGetValues may be used several times in one statement, but it does not contain the row add in the statement itself, which will cause some keys did not check.

What is changed and how it works?

Add an interface BatchGet to get the statement and the transaction key/values, and merge them with the storage BatchGet results.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

@jackysp
Copy link
Member Author

jackysp commented Feb 20, 2019

/run-all-tests

@codecov-io
Copy link

codecov-io commented Feb 20, 2019 •
edited
Loading

Codecov Report

Merging #9374 into master will increase coverage by 0.01%.
The diff coverage is 56.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9374      +/-   ##
==========================================
+ Coverage   67.17%   67.18%   +0.01%     
==========================================
  Files         373      373              
  Lines       78272    78291      +19     
==========================================
+ Hits        52578    52599      +21     
+ Misses      20990    20983       -7     
- Partials     4704     4709       +5
Impacted Files Coverage Δ
kv/txn.go 53.65% <ø> (+18.73%) ⬆️
kv/fault_injection.go 60% <0%> (-2.07%) ⬇️
kv/mock.go 68.53% <0%> (+0.76%) ⬆️
ddl/index.go 80.45% <100%> (ø) ⬆️
executor/admin.go 76.76% <100%> (ø) ⬆️
executor/batch_checker.go 79.01% <100%> (ø) ⬆️
store/tikv/txn.go 78.78% <59.09%> (-3.29%) ⬇️
session/txn.go 74.75% <65%> (-1.05%) ⬇️
ddl/delete_range.go 75.13% <0%> (-3.18%) ⬇️
executor/join.go 78.9% <0%> (-0.53%) ⬇️
... and 3 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 38a453d...02fbd1b. Read the comment docs.

@crazycs520 crazycs520 added the type/bugfix This PR fixes a bug. label Feb 21, 2019
crazycs520
crazycs520 reviewed Feb 21, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor

@tiancaiamao @winkyao PTAL

@jackysp
Copy link
Member Author

jackysp commented Feb 21, 2019

/run-all-tests

tiancaiamao
tiancaiamao reviewed Feb 21, 2019
View reviewed changes
executor/write_test.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Feb 21, 2019
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 21, 2019
@ngaut ngaut requested a review from disksing February 21, 2019 14:51
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao
Copy link
Contributor

/run-all-tests

@tiancaiamao tiancaiamao added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 22, 2019
@jackysp jackysp merged commit 18449d7 into pingcap:master Feb 22, 2019
jackysp added a commit to jackysp/tidb that referenced this pull request Feb 22, 2019
@jackysp
*: merge statement buffer when BatchGetValues (pingcap#9374)
286eecf
@jackysp jackysp mentioned this pull request Feb 22, 2019
Merged
jackysp added a commit to jackysp/tidb that referenced this pull request Feb 22, 2019
@jackysp
*: merge statement buffer when BatchGetValues (pingcap#9374)
0c6c226
@jackysp jackysp mentioned this pull request Feb 22, 2019
Merged
zimulala pushed a commit that referenced this pull request Feb 22, 2019
@jackysp @zimulala
*: merge statement buffer when BatchGetValues (#9374) (#9414)
108ce5b
zimulala pushed a commit that referenced this pull request Feb 22, 2019
@jackysp @zimulala
*: merge statement buffer when BatchGetValues (#9374) (#9416)
7aefd50
@jackysp jackysp deleted the merge_stmt_buf branch April 4, 2019 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

@crazycs520 crazycs520 crazycs520 left review comments

@tiancaiamao tiancaiamao tiancaiamao approved these changes

@disksing disksing disksing approved these changes

@lysu lysu Awaiting requested review from lysu

Assignees
No one assigned
Labels
component/session sig/execution SIG execution sig/transaction SIG:Transaction status/LGT3 The PR has already had 3 LGTM. 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
@jackysp @codecov-io @crazycs520 @tiancaiamao @zimulala @disksing