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

store/tikv: export failpoints #22539

Merged
merged 2 commits into from
Jan 27, 2021
Merged

store/tikv: export failpoints #22539

merged 2 commits into from
Jan 27, 2021

Conversation

andylokandy
Copy link
Contributor

What problem does this PR solve?

Issue Number: part of #22513

Problem Summary:

failpoints in store/tikv should be explicitly defined so as to be usable after store/tikv being moved to other repository.

What is changed and how it works?

What's Changed:

Added failponit.go in store/tikv which contains exposed failpoints. Also, the failpoints injection is expanded manually.

Check List

Tests

  • Unit test

Release note

  • No release note

@andylokandy andylokandy requested a review from a team as a code owner January 26, 2021 10:06
@andylokandy andylokandy requested review from lzmhhh123 and removed request for a team January 26, 2021 10:06
@andylokandy
Copy link
Contributor Author

andylokandy commented Jan 26, 2021

I exposed only a little amount of them to test if it works.

@andylokandy andylokandy added the sig/transaction SIG:Transaction label Jan 26, 2021
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jan 26, 2021
store/tikv/failpoint.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
disksing
disksing previously approved these changes Jan 27, 2021
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 27, 2021
@andylokandy
Copy link
Contributor Author

/run-tests

@@ -587,12 +587,12 @@ func extractLockFromKeyErr(keyErr *pb.KeyError) (*Lock, error) {
}

func extractKeyErr(keyErr *pb.KeyError) error {
failpoint.Inject("ErrMockRetryableOnly", func(val failpoint.Value) {
if val, err := MockRetryableErrorResp.Eval(); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it hurt performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May cause cache miss. But in Go, i think it's not really concerned.

@andylokandy
Copy link
Contributor Author

/run-all-tests

1 similar comment
@andylokandy
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

Why do we need to update go.mod?

@@ -2400,19 +2400,19 @@ func (s *testSerialSuite) TestSplitRegionTimeout(c *C) {
c.Assert(failpoint.Disable("github.com/pingcap/tidb/store/tikv/MockSplitRegionTimeout"), IsNil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use tikv.MockSplitRegionTimeout.Disable() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Andy Lok <andylokandy@hotmail.com>
@andylokandy
Copy link
Contributor Author

@AndreMouche @disksing PTAL~

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 27, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 27, 2021
@andylokandy
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 27, 2021
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 22465
  • 22153

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 871856d into pingcap:master Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants