fix: Make GetVersioned() available after doing LazyLoadVersion #358
Conversation
mutable_tree.go
Outdated
| @@ -438,7 +447,7 @@ func (tree *MutableTree) Rollback() { | |||
| func (tree *MutableTree) GetVersioned(key []byte, version int64) ( | |||
| index int64, value []byte, | |||
| ) { | |||
| if tree.versions[version] { | |||
| if !tree.allLoaded || tree.versions[version] { | |||
There was a problem hiding this comment.
I think this should be:
if tree.allLoaded && tree.versions[version]
In the function below as well.
Please confirm.
There was a problem hiding this comment.
My intention is to fix this to be able to get the version by access of db even when we do LazyLoadVersion.
If we have done LazyLoadVersion then tree.allLoaded would be false. There is a problem that existing code cannot get a version by GetVersioned even though the version is in db when LazyLoadVersion is done .
There was a problem hiding this comment.
I see, we want to try to load if we didn't use LoadVersion before.
There was a problem hiding this comment.
If a node started with LazyLoadVersion, what should this function do? Should it be able to give us only the last version that it loaded? Or should it access the DB and give any version to us if it's there?
There was a problem hiding this comment.
Exactly as you wrote - we should try to load it from DB. Maybe we could even do a small optimization to avoid loading if we already tried it:
exists, checked := tree.versions[version]
if (!tree.allLoaded && !checked) || exists {
...
This way, we avoid entering the load, if we already tried to do it and the he version didn't exist.
There was a problem hiding this comment.
That's a good point. I applied the checking to VersionExists and made other functions use it.
| return existingHash, version, nil | ||
| // If the version already exists, return an error as we're attempting to overwrite. | ||
| // However, the same hash means idempotent (i.e. no-op). | ||
| if existingHash != nil { |
There was a problem hiding this comment.
len(existingHash) == 0 is a right check we should us here. It checks both the nil value and an empty slice.
There was a problem hiding this comment.
I don't think so. Only nil should be checked here. When we do LazyLoadVersion, there is no versions information, so we need to access db to see if there is the version or not. I put this condition in order not to run the code below when there is no version in db. If it is an empty slice, we should process the logic below.
There was a problem hiding this comment.
OK, makes sense. could you update the comment in the code to add information about empty slice?
There was a problem hiding this comment.
The comment about empty slice is already below, but I'll add a comment about the condition existingHash != nil
|
@robert-zaremba I modified the checking code as you advised. I'd appreciate it if you review it again. |
|
Hi @egonspace - are you planning to merge this PR? |
|
I accidentally removed the branch and recreated the PR. I'd appreciate it if you could review it again even if it's too much trouble. |
We can use
LazyLoadVersionby #148.But if we start app with
LazyLoadVersionwe cannot query app state by specific height becauseMutableTree.GetVersionedreturns nil for other previous version after doingLazyLoadVersion.Our nodes are facing a slow booting up problem because of the large app data, and I want to hear your opinion about whether we should modify the code like this.