Skip to content

*: remove the transaction kv count limit#11141

Merged
jackysp merged 5 commits into
pingcap:masterfrom
jackysp:remove_count_limit
Aug 9, 2019
Merged

*: remove the transaction kv count limit#11141
jackysp merged 5 commits into
pingcap:masterfrom
jackysp:remove_count_limit

Conversation

@jackysp
Copy link
Copy Markdown
Contributor

@jackysp jackysp commented Jul 9, 2019

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

It is extremely easy to reach the transaction kv count limit for TiDB. Which make the user have to modify their applications.

What is changed and how it works?

Remove the transaction kv count limit, though keep the transaction size limit.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported variable/fields change

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 9, 2019 •
edited
Loading

Codecov Report

Merging #11141 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master    #11141   +/-   ##
=========================================
  Coverage   81.456%   81.456%           
=========================================
  Files          429       429           
  Lines        92839     92839           
=========================================
  Hits         75623     75623           
  Misses       11813     11813           
  Partials      5403      5403

@jackysp jackysp force-pushed the remove_count_limit branch from 27ccb4d to 3cd6fb5 Compare July 9, 2019 07:44
zhangjinpeng87
zhangjinpeng87 reviewed Jul 14, 2019
View reviewed changes
Comment thread executor/seqtest/seq_executor_test.go Outdated
Copy link
Copy Markdown
Contributor

@zhangjinpeng87 zhangjinpeng87 Jul 14, 2019

Choose a reason for hiding this comment

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

If this test is not used any more, please remove it directly.

Copy link
Copy Markdown
Contributor Author

@jackysp jackysp Jul 14, 2019

Choose a reason for hiding this comment

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

This PR is still WIP, plz wait.

zhangjinpeng87
zhangjinpeng87 reviewed Jul 14, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

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

AndreMouche
AndreMouche reviewed Jul 14, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

Any performance test?

@jackysp
Copy link
Copy Markdown
Contributor Author

jackysp commented Jul 14, 2019 •
edited
Loading

Any performance test?

Some tests are being done, but not specifically for performance. I think this pr will not improve performance. We will do some profiling. However, these two days have been suspended because of the talent plan. @AndreMouche

@disksing
Copy link
Copy Markdown
Contributor

disksing commented Jul 16, 2019

Maybe use some kind of row count limit? It will be understandable than kv count limit.

@jackysp jackysp force-pushed the remove_count_limit branch from 3cd6fb5 to 2480c73 Compare July 31, 2019 02:39
@zz-jason
Copy link
Copy Markdown
Member

zz-jason commented Aug 3, 2019

@jackysp any update? do we need to continue processing this PR?

@jackysp
Copy link
Copy Markdown
Contributor Author

jackysp commented Aug 6, 2019

Yes, it is still WIP. I'm doing some tests for it, but the results may not be suitable to report to Github.

jackysp added 2 commits August 9, 2019 15:37
@jackysp
*: remove the transaction kv count limit
d4b6292 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
fix fmt
f51bcc0 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp force-pushed the remove_count_limit branch from 2480c73 to f51bcc0 Compare August 9, 2019 07:37
@jackysp
update test case
88ee18a 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp marked this pull request as ready for review August 9, 2019 08:04
@jackysp
Copy link
Copy Markdown
Contributor Author

jackysp commented Aug 9, 2019

/run-all-tests

@coocood
Copy link
Copy Markdown
Member

coocood commented Aug 9, 2019

LGTM

@jackysp jackysp added status/LGT2 Indicates that a PR has LGTM 2. sig/transaction SIG:Transaction and removed sig/transaction SIG:Transaction labels Aug 9, 2019
lysu
lysu approved these changes Aug 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu lysu added status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 9, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 9, 2019

/run-all-tests

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 9, 2019

@jackysp merge failed.

@jackysp
Copy link
Copy Markdown
Contributor Author

jackysp commented Aug 9, 2019

The unit test failed with the same reason as #11693.

@jackysp
Copy link
Copy Markdown
Contributor Author

jackysp commented Aug 9, 2019

/run-unit-test

1 similar comment
@jackysp
Copy link
Copy Markdown
Contributor Author

jackysp commented Aug 9, 2019

/run-unit-test

@jackysp jackysp merged commit df2075d into pingcap:master Aug 9, 2019
@jackysp jackysp mentioned this pull request Sep 6, 2019
Merged
@jackysp jackysp deleted the remove_count_limit branch February 27, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndreMouche AndreMouche AndreMouche left review comments

@zhangjinpeng87 zhangjinpeng87 zhangjinpeng87 left review comments

+1 more reviewer

@lysu lysu lysu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jackysp @disksing @zz-jason @coocood @sre-bot @lysu @AndreMouche @zhangjinpeng87