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: update kvrpc.Cleanup proto and change its behaviour #12212

Merged
merged 15 commits into from
Sep 23, 2019

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Check the TTL on the primary lock to decide the real status of a transaction.
After we update the TTL of a transaction, the TTL information on the secondary lock is not accurate.

What is changed and how it works?

Before this PR, Cleanup always rollbacks a transaction if it's not committed.
After this PR, Cleanup will not rollback a transaction if the primary lock is active.

Check List

Tests

  • Unit test

Side effects

  • Breaking backward compatibility

The semantics of Cleanup API change.

Release note

  • Write release note for bug-fix or new feature.

Before this PR, Cleanup always rollback a transaction if it's not committed.
After this PR, Cleanup will not rollback a transaction if the lock is active.
@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12212   +/-   ##
===========================================
  Coverage   81.2941%   81.2941%           
===========================================
  Files           454        454           
  Lines        100033     100033           
===========================================
  Hits          81321      81321           
  Misses        12926      12926           
  Partials       5786       5786

@tiancaiamao
Copy link
Contributor Author

PTAL @MyonKeminta @coocood @lysu

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

store/mockstore/mocktikv/mvcc_leveldb.go Outdated Show resolved Hide resolved
store/mockstore/mocktikv/mvcc_leveldb.go Show resolved Hide resolved
store/mockstore/mocktikv/mvcc_leveldb.go Outdated Show resolved Hide resolved
store/tikv/lock_resolver.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor Author

/run-all-tests tikv=pr/5471

@coocood
Copy link
Member

coocood commented Sep 18, 2019

LGTM

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tikv=pr/5471

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tikv=pr/5471

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tikv=pr/5471

@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @youjiali1995 @MyonKeminta

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

@coocood
Copy link
Member

coocood commented Sep 23, 2019

LGTM

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. But I hope if there are more comments in ResolveLocks' logic. I found it difficult to understand.

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

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

/run-unit-test

@tiancaiamao tiancaiamao merged commit 48557f7 into pingcap:master Sep 23, 2019
@tiancaiamao tiancaiamao deleted the clean-up branch September 23, 2019 11:45
@sre-bot
Copy link
Contributor

sre-bot commented Sep 23, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @tiancaiamao PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants