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

terror: provide a RegisterFinish API #1352

Merged
merged 3 commits into from
Sep 30, 2021
Merged

Conversation

tiancaiamao
Copy link
Collaborator

What problem does this PR solve?

Prevent regression like pingcap/tidb#28190

What is changed and how it works?

TiDB will register all the error kinds in its initialization, and then call this API
After that, misuse of the terror will panic, thus the bug is exposed early.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

In TiDB

Code changes

  • Has exported function/method change

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 27, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • ichn-hu
  • kennytm

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@tiancaiamao
Copy link
Collaborator Author

pingcap/tidb#28438

terror/terror.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ichn-hu ichn-hu left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: kennytm <kennytm@gmail.com>
@tiancaiamao
Copy link
Collaborator Author

My main point is actually here pingcap/tidb#25405 (comment)

Is there some way to prevent this kind of error in the future?
Maybe nobody read the document and maybe people still make this kind of mistakes in the future.

We have got stuck on the error before, and fixed it then.
But later a contributor file a PR and misuse the terror package again...
This time we're not so lucky, the reviewers don't catch it and our customer trigger the bug

@ti-chi-bot ti-chi-bot added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Sep 30, 2021
@tangenta
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: bd914f4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants