-
Notifications
You must be signed in to change notification settings - Fork 264
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: speed up rollback command #620
Conversation
* skip deleteNodesFrom * lazyLoad latest version
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.
Left few comments.
if !fastMode { | ||
latestVersion, err = tree.LoadVersion(targetVersion) | ||
} else { | ||
latestVersion, err = tree.LazyLoadVersion(targetVersion) |
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.
There is a param name clash:
- seams that we want to use
fastMode
when we don't want to usefastCache
- so let's rename
fastMode
parameter to something different, eglazy bool
ornoFastCache bool
.
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.
Actually this flag is mainly related with rollback, should we rename to FastRollback
or OfflineRollback
.
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.
OK, but in the DeleteVersionsFrom
it's related to the fast cache
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.
Make sense, then OfflineRollback
might better since assumption is based on offline and re-index on restart.
mutable_tree.go
Outdated
return tree.LoadVersionForOverwritingWithMode(targetVersion, false) | ||
} | ||
|
||
func (tree *MutableTree) LoadVersionForOverwritingWithMode(targetVersion int64, fastMode bool) (int64, error) { |
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.
let's add a doc string explaining the new parameter.
nodedb.go
Outdated
@@ -420,7 +420,7 @@ func (ndb *nodeDB) DeleteVersion(version int64, checkLatestVersion bool) error { | |||
} | |||
|
|||
// DeleteVersionsFrom permanently deletes all tree versions from the given version upwards. | |||
func (ndb *nodeDB) DeleteVersionsFrom(version int64) error { | |||
func (ndb *nodeDB) DeleteVersionsFrom(version int64, fastMode bool) error { |
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.
ditto, also update doc string
nodedb.go
Outdated
}) | ||
|
||
return nil | ||
}) |
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 inner function is the same in both cases. Let's avoid copy-paste.
nodedb.go
Outdated
return nil | ||
}) | ||
} else { | ||
err = ndb.traverseOrphansVersion(version-1, func(key, hash []byte) error { |
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.
why do we change a version here?
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.
since toVersion
in orphan records is current version-1
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.
thanks, let's add a comment in the code.
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.
pre-approving. let's finalize the naming
if !fastMode { | ||
latestVersion, err = tree.LoadVersion(targetVersion) | ||
} else { | ||
latestVersion, err = tree.LazyLoadVersion(targetVersion) |
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.
OK, but in the DeleteVersionsFrom
it's related to the fast cache
I did a cleaner version: #636 |
if err := tree.enableFastStorageAndCommitLocked(); err != nil { | ||
return latestVersion, err | ||
} | ||
} else if err = tree.ndb.Commit(); err != nil { |
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.
@yihuang seems we still need commit if skipFastStorageUpgrade
?
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.
I see, so it don't commit previously?
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.
yup, I wonder if need fix as a separate bug
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.
I see, the current rollback cmd maybe don't work at all if the fast node is disabled 😂
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.
Closing this PR, let's limit the options.
Update