Skip to content
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 #636

Merged
merged 2 commits into from
Dec 12, 2022
Merged

feat: speed up rollback command #636

merged 2 commits into from
Dec 12, 2022

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Nov 30, 2022

  • fully rebuild fast node indexes after rollback, rather than leave it in dirty state.
  • skip unnecessary subtree traversal when deleting the latest version
  • restrict the iteration range when deleting orphans
  • support LazyLoadVersion, remove lazy support, we might be able to use lazy mode by default

A cleaner version based on: #636

Test result:

Combined with the lazy loading option, disabling fastnode, rollback takes 1.8s on one of our testnet archive node(337G), previously it was several hours.

@@ -605,6 +586,11 @@ func (ndb *nodeDB) deleteNodesFrom(version int64, hash []byte) error {
return err
}

if node.version < version {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like

nodedb.go Outdated Show resolved Hide resolved
util.go Outdated Show resolved Hide resolved
nodedb.go Outdated Show resolved Hide resolved
@yihuang yihuang requested review from cool-develope and mmsqe and removed request for cool-develope and mmsqe December 6, 2022 01:47
- fully rebuild fast node indexes after rollback, rather than leave it in dirty state.
- skip unnecessary subtree traversal when deleting the latest version.
- restrict the iteration range when deleting orphans.
if err != nil {
return err
}
// NOTICE: we don't touch fast node indexes here, because it'll be rebuilt later because of version mismatch.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the trade off here?

Copy link
Collaborator Author

@yihuang yihuang Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the previous logic to just delete some fastnodes is simply wrong, because fast node is assumed to be represent the complete live state, it's wrong to leave a partial fast node index.

In new version, fast node index will be fully rebuilt because of version mismatch, and if user don't want to slow down rollback, they can disable fast node index before rollback, after recovered the node operation, user can find another good time to rebuild the fast node index by enable it again.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utack. Should we backport this?

@yihuang
Copy link
Collaborator Author

yihuang commented Dec 12, 2022

Utack. Should we backport this?

I hope so, it's a big UX improvement for existing version.

@tac0turtle tac0turtle merged commit d4086fe into cosmos:master Dec 12, 2022
@tac0turtle
Copy link
Member

@Mergifyio backport release/v0.19.x

mergify bot pushed a commit that referenced this pull request Dec 12, 2022
(cherry picked from commit d4086fe)

# Conflicts:
#	nodedb.go
@mergify
Copy link
Contributor

mergify bot commented Dec 12, 2022

backport release/v0.19.x

✅ Backports have been created

tac0turtle added a commit that referenced this pull request Dec 26, 2022
Co-authored-by: yihuang <huang@crypto.com>
Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>
ankurdotb pushed a commit to cheqd/iavl that referenced this pull request Feb 28, 2023
Co-authored-by: yihuang <huang@crypto.com>
Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants