-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: fix lockTTL too large if local time is behind timestamp. #13865
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13865 +/- ##
===============================================
- Coverage 81.0525% 80.117% -0.9356%
===============================================
Files 478 474 -4
Lines 124270 117085 -7185
===============================================
- Hits 100724 93805 -6919
+ Misses 16053 15852 -201
+ Partials 7493 7428 -65 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
store/tikv/2pc_test.go
Outdated
txn.startTS = oracle.ComposeTS(oracle.GetPhysical(time.Now().Add(time.Second*10)), 1) | ||
txn.SetOption(kv.Pessimistic, true) | ||
time.Sleep(time.Millisecond * 100) | ||
err := txn.LockKeys(context.Background(), nil, txn.startTS, kv.LockAlwaysWait, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous code also includes the problem that forUpdateTS
is used to calculate TTL by mistake. Need we use a bit larger forUpdateTS
here?
txn := s.begin(c) | ||
txn.startTS = oracle.ComposeTS(oracle.GetPhysical(time.Now().Add(time.Second*10)), 1) | ||
txn.SetOption(kv.Pessimistic, true) | ||
time.Sleep(time.Millisecond * 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem it is not a stability test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ttl check is relaxed to 50ms delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-unit-test |
LGTM |
/merge |
/run-all-tests |
cherry pick to release-3.0 failed |
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. @coocood PTAL. |
What problem does this PR solve?
elapsed time should only be calculated by local time, otherwise it may be negative and cast to uint64, a value that is too large.
What is changed and how it works?
Use
txn.startTime
to calculate elapsed time.Check List
Tests
Related changes
Release note