-
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: make snapshot thread safe (#17593) #18145
Conversation
@coocood,Thanks for you review. |
@tiancaiamao |
Move code from This code is not safe, as txn maybe set to
I override GetSnapshot to provide the snapshot. |
Link issue #16817 |
/run-all-tests |
github is unavailable |
LGTM |
@coocood, Thanks for your review, however we are sorry that your vote won't be count. You already give a LGTM to this PR |
@bobotu PTAL |
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.
Not in a atomic way, but it should work , as a pointer is in a machine word size (assumption on hardware), either nil or a kv.Snapshot is OK.
This may have a problem with ARMv7?
In particular, the model was originally non-multicopy-atomic: writes could become visible to some other threads before becoming visible to all
Anyway, it isn't a blocker for this PR.
LGTM
LGTM |
@coocood, Thanks for your review, however we are sorry that your vote won't be count. You already give a LGTM to this PR |
@SunRunAway,Thanks for you review. |
/merge |
2 similar comments
/merge |
/merge |
/run-all-tests |
@tiancaiamao merge failed. |
/run-common-test |
/run-check_dev |
What problem does this PR solve?
Cherry pick #17593 to release-4.0
Problem Summary:
A correct fix need to cherry pick a series of commits:
#16056
#17555
#17593
#17678
#17911
That maybe too much for 4.0
What is changed and how it works?
make snapshot thread safe
Check List
Tests
Release note