Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(store/v2): handle store upgrades in RootStore #18277
feat(store/v2): handle store upgrades in RootStore #18277
Changes from 15 commits
975f8fe
22b68b4
e245fc6
7e180f1
2a315f8
9d99295
8c2d1da
302f7a3
8d36583
f5c7285
a4fdf45
52bda2a
d83b28e
d527b2e
abf887d
b32979d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 function
LastCommitID
is returning a newCommitID
with only the latest version set whenlastCommitInfo
isnil
. This might lead to unexpected behavior if the caller expects a fully populatedCommitID
. Consider returning an error in this case.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.
It is a WIP? couldn't see the call of
loadVersion
with upgrades.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.
This implementation doesn't support store upgrades since it's all a single tree/store key. In other words, store keys are ignored in the single tree implementation.
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 function
loadVersion
is ignoring theStoreUpgrades
parameter. If this is intentional, consider documenting this behavior in the function comment to avoid confusion.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 function
Commit
is resetting therootKVStore
to the current version after committing the changes. This might lead to data loss if there are uncommitted changes in therootKVStore
. Consider adding a check to ensure that there are no uncommitted changes before resetting therootKVStore
.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 function
writeSC
is creating a newCommitInfo
with only oneStoreInfo
for the default store key. This might lead to unexpected behavior if the caller expects aCommitInfo
with all theStoreInfo
s. Consider adding a check to ensure that all theStoreInfo
s are included in theCommitInfo
.