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: when commit get a CommitTsExpired error, retry with a new commitTS #12980

Merged
merged 10 commits into from
Oct 30, 2019

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Before we can use non-block read for large transaction, we have to handle the commitTS problem:

We must guarantee the invariance that commitTS > minCommitTS

There is an easiest way to handle that:
TiKV return a retryable error and TiDB retrying with a newer commitTS.

What is changed and how it works?

In 2PC commit phase, if TiKV return a CommitTsExpired error, TiDB get a new commitTS and retry.

Check List

Tests

Side effects

  • Breaking backward compatibility

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12980   +/-   ##
===========================================
  Coverage   80.1441%   80.1441%           
===========================================
  Files           468        468           
  Lines        109857     109857           
===========================================
  Hits          88044      88044           
  Misses        15106      15106           
  Partials       6707       6707

store/tikv/2pc.go Outdated Show resolved Hide resolved
@coocood
Copy link
Member

coocood commented Oct 29, 2019

LGTM

c.mu.Lock()
c.commitTS = commitTS
c.mu.Unlock()
return c.commitKeys(bo, batch.keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kinda afraid that recursion may cause troubles...

Copy link
Contributor Author

@tiancaiamao tiancaiamao Oct 30, 2019

Choose a reason for hiding this comment

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

Retry two many times and stack overflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The old code also take this pattern
  2. If it's really a problem, we can refactor the code with another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure to do we need a backoff here before recursion retry - -? @tiancaiamao if so I will add it at #16061 later

store/tikv/2pc_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood
Copy link
Member

coocood commented Oct 30, 2019

@youjiali1995 PTAL

Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

LGTM

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

1 similar comment
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao tiancaiamao added the status/LGT3 The PR has already had 3 LGTM. label Oct 30, 2019
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@coocood coocood added the status/can-merge Indicates a PR has been approved by a committer. label Oct 30, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 30, 2019

Your auto merge job has been accepted, waiting for 13021

@sre-bot
Copy link
Contributor

sre-bot commented Oct 30, 2019

/run-all-tests

@sre-bot sre-bot merged commit 7fe81b7 into pingcap:master Oct 30, 2019
@tiancaiamao tiancaiamao deleted the retry-commit-ts branch October 30, 2019 09:23
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv sig/transaction SIG:Transaction 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.

6 participants