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

*: add auto_random id cache for statement retrying and table recover #14711

Merged
merged 13 commits into from
Feb 19, 2020

Conversation

tangenta
Copy link
Contributor

What problem does this PR solve?

This is a patch for #13127.

What is changed and how it works?

Add retry and recover table cache support for auto_random.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release note

  • add auto_random id cache for statement retrying and table recover


func (r *retryInfoAutoIDs) next() (int64, error) {
if r.currentOffset >= len(r.autoIDs) {
return 0, errCantGetValidID
Copy link
Member

@bb7133 bb7133 Feb 10, 2020

Choose a reason for hiding this comment

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

The error message of errCantGetValidID is "cannot get valid auto-increment id in retry". I suggest to modify it to "Cannot get a valid auto-ID when retrying the statement", what do you think?

Copy link
Contributor

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

sessionctx/variable/session.go Show resolved Hide resolved

// AddAutoIncrementID adds id to autoIncrementIDs.
func (r *RetryInfo) AddAutoIncrementID(id int64) {
r.autoIncrementIDs.add(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think should be directly r.autoRandomIDs.autoIDs = append(r.autoRandomIDs.autoIDs, id) here.
AddAutoIncrementID()->add()->append() may be too deep.

ddl/ddl.go Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
@tangenta
Copy link
Contributor Author

/run-check_dev_2

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.

No test for those features?

ddl/ddl_worker.go Outdated Show resolved Hide resolved
executor/ddl.go Outdated Show resolved Hide resolved
@reafans
Copy link
Contributor

reafans commented Feb 17, 2020

lgtm

@reafans reafans added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 17, 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/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 18, 2020
Copy link
Contributor

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

ddl/table.go Outdated Show resolved Hide resolved
executor/seqtest/seq_executor_test.go Outdated Show resolved Hide resolved
@AilinKid
Copy link
Contributor

should resolve the conflict.

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@AilinKid AilinKid added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 18, 2020
@AilinKid
Copy link
Contributor

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 18, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 18, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 18, 2020

@tangenta merge failed.

@bb7133
Copy link
Member

bb7133 commented Feb 19, 2020

/run-unit-test

@AilinKid
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Feb 19, 2020

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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