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
Fix Restart application issue #5579
Fix Restart application issue #5579
Changes from 17 commits
a540b0e
253ad5c
f1e5ba9
923f1e5
a35591b
0ae90d8
d612e56
63bd983
9e4b434
fbd1a52
42fc8ef
aa81b5d
67d5087
c9c0fd1
f041162
d52da2c
f52fa5e
5fab6cd
c811093
4fa7fe7
e8676ab
d8186a6
b210d0e
5b00d1a
5102a90
6e67a1b
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.
This seems a little dangerous.
st.tree
will flush or not based on some pruning options it received.Then we ask
st.prunning
whether it flushed.These are two different pieces of logic that need to be kept in sync for this to work (and errors may be subtle and very hard to detect until a restart).
Why not add this info
tree.SaveVersion
, so it says "yes, I saved it and did/did not flush it"? This guarantees to keep in syncThere 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.
@AdityaSripal do you have a response to this? Or is this even valid?
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.
Yes I agree, didn't add this info to
tree.SaveVersion
because I wanted to keep the IAVL API intact.Doing this would require a breaking change to IAVL in a new major version. Don't want this PR to hold up on that so i think this solution works for now. And we can create an issue to do this on iAVL
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.
Maybe
previous > 0
is "safer".Though I cannot think of a scenario when this could be negative, still better safe
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.
How about
if err != nil && !iavl.ErrVersionDoesNotExist.Is(err)
https://github.com/cosmos/cosmos-sdk/blob/master/types/errors/errors.go#L175
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.
++