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

*: support auto_random table option #16750

Merged
merged 17 commits into from
Apr 24, 2020
Merged

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Apr 23, 2020

What problem does this PR solve?

Problem Summary: support auto_random_base table option, this PR is related to issue pingcap/br#241

What is changed and how it works?

How it Works:
After this table option:
We can do the auto_random rebase by alter table xxx auto_random_base = num
Also we can specify the initial auto_random base by create table t xxx(xxx) auto_random_base = num

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

support auto_random table option for rebase auto_random allocator

@AilinKid AilinKid requested a review from a team as a code owner April 23, 2020 06:25
@ghost ghost requested review from SunRunAway and removed request for a team April 23, 2020 06:25
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Apr 23, 2020
@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master     #16750   +/-   ##
===========================================
  Coverage   80.5344%   80.5344%           
===========================================
  Files           507        507           
  Lines        138491     138491           
===========================================
  Hits         111533     111533           
  Misses        18299      18299           
  Partials       8659       8659           

types/parser_driver/special_cmt_ctrl.go Outdated Show resolved Hide resolved
table/tables/tables.go Show resolved Hide resolved
@@ -74,7 +74,7 @@ func (b *Builder) ApplyDiff(m *meta.Meta, diff *model.SchemaDiff) ([]int64, erro
// We try to reuse the old allocator, so the cached auto ID can be reused.
var allocs autoid.Allocators
if tableIDIsValid(oldTableID) {
if oldTableID == newTableID && diff.Type != model.ActionRenameTable && diff.Type != model.ActionRebaseAutoID && diff.Type != model.ActionModifyTableAutoIdCache {
if oldTableID == newTableID && diff.Type != model.ActionRenameTable && diff.Type != model.ActionRebaseAutoID && diff.Type != model.ActionRebaseAutoRandomBase && diff.Type != model.ActionModifyTableAutoIdCache {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that if we only rebase auto_random, but auto_id also rebase(we will clean the cache in TiDB).

Copy link
Contributor Author

@AilinKid AilinKid Apr 24, 2020

Choose a reason for hiding this comment

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

seems right. should keep the special allocator survive?

Copy link
Contributor

@zimulala zimulala Apr 24, 2020

Choose a reason for hiding this comment

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

Could we handle it?I think it feels confusing to users. And please add some tests.

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, Addressed

@AilinKid
Copy link
Contributor Author

/run-integration-copr-test

@AilinKid
Copy link
Contributor Author

/run-integration-common-test

1 similar comment
@AilinKid
Copy link
Contributor Author

/run-integration-common-test

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

/run-integration-common-test

@bb7133 bb7133 added needs-cherry-pick-3.1 require-LGT3 Indicates that the PR requires three LGTM. labels Apr 24, 2020
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/LGT3 The PR has already had 3 LGTM. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Apr 24, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 24, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Apr 24, 2020

@AilinKid merge failed.

@bb7133
Copy link
Member

bb7133 commented Apr 24, 2020

/run-integration-common-test
/run-integration-copr-test

@bb7133 bb7133 merged commit 7b25ce0 into pingcap:master Apr 24, 2020
@AilinKid
Copy link
Contributor Author

integration-common-test seems nothing to do with this PR, so we gonna merge it now

@sre-bot
Copy link
Contributor

sre-bot commented Apr 24, 2020

cherry pick to release-3.1 in PR #16812

@sre-bot
Copy link
Contributor

sre-bot commented Apr 24, 2020

cherry pick to release-4.0 in PR #16813

AilinKid added a commit to sre-bot/tidb that referenced this pull request Apr 26, 2020
AilinKid added a commit to sre-bot/tidb that referenced this pull request Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants