-
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
Changes from all commits
dc90e0b
a9d704d
325405d
76c0d19
1493ae5
e0cfa42
36f3727
2e9db23
255d416
e3f445f
06baf1f
7e88c5c
9b36a27
d64ee05
1f7dd20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -616,19 +616,33 @@ func (tree *MutableTree) LoadVersion(targetVersion int64) (int64, error) { | |
// LoadVersionForOverwriting attempts to load a tree at a previously committed | ||
// version, or the latest version below it. Any versions greater than targetVersion will be deleted. | ||
func (tree *MutableTree) LoadVersionForOverwriting(targetVersion int64) (int64, error) { | ||
latestVersion, err := tree.LoadVersion(targetVersion) | ||
return tree.LoadVersionForOverwritingWithMode(targetVersion, false) | ||
} | ||
|
||
// LoadVersionForOverwritingWithMode call LoadVersionForOverwriting with offlineRollback | ||
// to allow rollback in a quick and dirty way. | ||
func (tree *MutableTree) LoadVersionForOverwritingWithMode(targetVersion int64, offlineRollback bool) (int64, error) { | ||
var latestVersion int64 | ||
var err error | ||
if !offlineRollback { | ||
latestVersion, err = tree.LoadVersion(targetVersion) | ||
} else { | ||
latestVersion, err = tree.LazyLoadVersion(targetVersion) | ||
} | ||
if err != nil { | ||
return latestVersion, err | ||
} | ||
|
||
if err = tree.ndb.DeleteVersionsFrom(targetVersion + 1); err != nil { | ||
if err = tree.ndb.DeleteVersionsFrom(targetVersion+1, offlineRollback); err != nil { | ||
return latestVersion, err | ||
} | ||
|
||
if !tree.skipFastStorageUpgrade { | ||
if !tree.skipFastStorageUpgrade && !offlineRollback { | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. @yihuang seems we still need commit if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Closing this PR, let's limit the options. |
||
return latestVersion, err | ||
} | ||
|
||
tree.ndb.resetLatestVersion(latestVersion) | ||
|
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:
fastMode
when we don't want to usefastCache
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
orOfflineRollback
.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 cacheThere 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.