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

ddl: fix error code and error name #13329

Merged
merged 9 commits into from Nov 12, 2019
Merged

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Nov 10, 2019

What problem does this PR solve?

This PR refines all Errors defined in ddl/ddl.go:

  • Assign specific codes for each error to prevent 1105(Unknown) error code.
  • Assign code 8060 for all errors caused by invalid DDL object state, for example, ErrInvalidTableState and ErrInvalidDatabaseState shares error code 8060.
  • Assign code 8050 for most errors that caused by the operations that TiDB not supported yet, for example, errRunMultiSchemaChanges and errCantDropColWithIndex.

Related Parser PR: pingcap/parser#630

What is changed and how it works?

NA

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

Side effects

  • Breaking backward compatibility

@bb7133
Copy link
Member Author

bb7133 commented Nov 10, 2019

PTAL @jackysp

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

ddl/ddl.go Outdated
// ErrInvalidTableState returns for invalid Table state.
ErrInvalidTableState = terror.ClassDDL.New(codeInvalidTableState, "invalid table state")
ErrInvalidTableState = terror.ClassDDL.New(mysql.ErrInvalidDDLState, fmt.Sprintf(mysql.MySQLErrName[mysql.ErrInvalidDDLState], "table"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change the way it is used?
Now we use it like ErrInvalidTableState.GenWithStack("invalid table state %v", tbInfo.State). I think we can use it like ErrInvalidTableState.GenWithStackByArgs(...) in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 12, 2019
@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13329   +/-   ##
===========================================
  Coverage   80.3502%   80.3502%           
===========================================
  Files           469        469           
  Lines        113793     113793           
===========================================
  Hits          91433      91433           
  Misses        15309      15309           
  Partials       7051       7051

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

@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 Nov 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

/run-all-tests

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

Approve

@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

@bb7133 merge failed.

Copy link
Member

@wjhuang2016 wjhuang2016 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
Copy link
Member Author

bb7133 commented Nov 12, 2019

mysql-test need to be updated as well

@bb7133
Copy link
Member Author

bb7133 commented Nov 12, 2019

/run-all-tests tidb-test=pr/945

@bb7133 bb7133 added status/all tests passed and removed status/can-merge Indicates a PR has been approved by a committer. labels Nov 12, 2019
@bb7133 bb7133 merged commit 5886055 into pingcap:master Nov 12, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@bb7133 bb7133 deleted the bb7133/fix_ddl_errcode branch December 29, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT3 The PR has already had 3 LGTM. type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants