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

*: using standard error to replace terror #19425

Merged
merged 9 commits into from
Sep 8, 2020
Merged

Conversation

imtbkcat
Copy link

What problem does this PR solve?

Problem Summary: Replace error in tidb with standard error. parser pr is: pingcap/parser#982

What is changed and how it works?

Proposal: Standardize error codes and messages

What's Changed:
Change error message in test cases.

How it Works:
Error message changed after terror.Error has been replaced with errors.Error

Related changes

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

Check List

Tests

  • Unit test

Side effects

  • Breaking backward compatibility

Release note

  • replace error code and message with standard error.

@imtbkcat imtbkcat added the type/enhancement The issue or PR belongs to an enhancement. label Aug 25, 2020
@imtbkcat imtbkcat requested review from a team as code owners August 25, 2020 05:22
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

@imtbkcat please fix ci.

@imtbkcat imtbkcat force-pushed the stderr branch 3 times, most recently from 424337b to 6bd9443 Compare September 3, 2020 05:55
@@ -414,8 +415,8 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job)
job.Error = toTError(err)
}
if !job.Error.Equal(errCancelledDDLJob) {
job.Error = job.Error.Class().Synthesize(job.Error.Code(),
fmt.Sprintf("DDL job rollback, error msg: %s", job.Error.ToSQLError().Message))
job.Error = terror.GetErrClass(job.Error).Synthesize(terror.ErrCode(job.Error.Code()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use errors.Annotatef here directly? @wjhuang2016

Copy link
Author

Choose a reason for hiding this comment

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

Here job.Error should be a terror.Errors type, but errors.Annotatef will produce an error type.

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

Don't forget to change the parser dependence in the go.mod.

@imtbkcat
Copy link
Author

imtbkcat commented Sep 7, 2020

PTAL @bb7133 @cfzjywxk @SunRunAway

Copy link
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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2020
@imtbkcat imtbkcat added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Sep 8, 2020
@imtbkcat imtbkcat added this to the v4.0.6 milestone Sep 8, 2020
@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

This pr can not update parser now, because of #19709 cause CI fail

@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/run-all-tests

1 similar comment
@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/run-all-tests

Copy link
Contributor

@bobotu bobotu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot
Copy link
Contributor

@bobotu,Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments.See the corresponding SIG page for more information. Related SIGs: execution(slack),ddl(slack),planner(slack).

@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/run-all-tests

@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/merge

@ti-srebot
Copy link
Contributor

@imtbkcat Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

@bobotu
Copy link
Contributor

bobotu commented Sep 8, 2020

/run-all-tests

@bobotu
Copy link
Contributor

bobotu commented Sep 8, 2020

/run-unit-test

1 similar comment
@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/run-unit-test

@SunRunAway SunRunAway removed their request for review September 8, 2020 12:01
@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/run-unit-test

@imtbkcat imtbkcat merged commit 449587a into pingcap:master Sep 8, 2020
@imtbkcat
Copy link
Author

imtbkcat commented Sep 8, 2020

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 8, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19888

@imtbkcat
Copy link
Author

/run-cherry-picker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants