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

meta: fix the allocator batch size compute logic #17271

Merged
merged 6 commits into from
Jun 1, 2020

Conversation

AilinKid
Copy link
Contributor

What problem does this PR solve?

Problem Summary: fix logic in allocator batch size computation

What is changed and how it works?

What's Changed:
when the local cache size if not enough for allocN, do the new batchSize computation based on the global new base in the txn.

so we postpone the NextStep adjustment to meta txn and store it after that.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Release note

  • meta: fix the allocator batch size compute logic

@AilinKid AilinKid added type/bugfix This PR fixes a bug. status/WIP sig/sql-infra SIG: SQL Infra labels May 18, 2020
@AilinKid
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master     #17271   +/-   ##
===========================================
  Coverage   79.8700%   79.8700%           
===========================================
  Files           520        520           
  Lines        140005     140005           
===========================================
  Hits         111822     111822           
  Misses        19228      19228           
  Partials       8955       8955           

Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@tangenta tangenta added the status/LGT1 Indicates that a PR has LGTM 1. label May 19, 2020
@AilinKid AilinKid requested a review from djshow832 May 20, 2020 02:47
@AilinKid
Copy link
Contributor Author

/run-all-tests

n1 = CalcNeededBatchSize(newBase, int64(n), increment, offset, alloc.isUnsigned)
// Although the step is customized by user, we still need to make sure nextStep is big enough for insert batch.
if nextStep < n1 {
nextStep = n1
Copy link
Contributor

Choose a reason for hiding this comment

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

It was n1*2 before but it's n1 now. Is that right?

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, allocate what you have asked for is correct

Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added this to the v3.0.15 milestone May 29, 2020
@bb7133 bb7133 added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label May 29, 2020
@AilinKid AilinKid force-pushed the autoid_fix_batch_size_logic branch from c8a5200 to 1922f0e Compare June 1, 2020 02:30
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 1, 2020
@AilinKid
Copy link
Contributor Author

AilinKid commented Jun 1, 2020

/run-all-tests

@AilinKid
Copy link
Contributor Author

AilinKid commented Jun 1, 2020

/run-integration-copr-test

@AilinKid AilinKid merged commit 9162cfa into pingcap:master Jun 1, 2020
@AilinKid
Copy link
Contributor Author

AilinKid commented Jun 1, 2020

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 1, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

cherry pick to release-2.1 in PR #17547

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 1, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

cherry pick to release-3.0 in PR #17548

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 1, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

cherry pick to release-3.1 in PR #17549

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 1, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 1, 2020

cherry pick to release-4.0 in PR #17550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants