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.

Sign up for GitHub

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

Jump to bottom

feat: Uses BatchWithFlusher in iavl tree #807

Merged
merged 4 commits into from
Aug 11, 2023
Merged

feat: Uses BatchWithFlusher in iavl tree #807

merged 4 commits into from
Aug 11, 2023

Conversation

catShaark
Copy link
Contributor

This PR replace the use of normal dbm.Batch with BatchWithFlusher in iavl tree
It also hard code the value of flushthreshold to 100KB

@catShaark catShaark requested a review from a team as a code owner August 8, 2023 17:20
@catShaark catShaark mentioned this pull request Aug 8, 2023
Closed
@catShaark catShaark changed the title Uses BatchWithFlusher in iavl tree Feat: Uses BatchWithFlusher in iavl tree Aug 8, 2023
@catShaark catShaark changed the title Feat: Uses BatchWithFlusher in iavl tree feat: Uses BatchWithFlusher in iavl tree Aug 8, 2023
tac0turtle
tac0turtle approved these changes Aug 10, 2023
View reviewed changes
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.

LGTM thank you

cool-develope
cool-develope approved these changes Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@cool-develope cool-develope left a comment

Choose a reason for hiding this comment

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

overall looks good to me, except why hard code the flushThreshold?

@catShaark
Copy link
Contributor Author

catShaark commented Aug 10, 2023 •
edited
Loading

That's because I discussed with marko and we decide to hard code the value to be the best option based on benchmark result run on osmosis node

@julienrbrt
Copy link
Member

+1 on @cool-develope comment.

Given that newNodeDB takes Options we could add an option in here: https://github.com/notional-labs/iavl/blob/a945f5f903800a214d30e55f87ca96cc6bc78051/options.go#L71-L89 and use 100000 in the DefaultOptions. If going this way, we should make them variadic so you don't end up overwriting that value to 0 by defining new options.

@tac0turtle
Copy link
Member

@julienrbrt you fine with merging this and then i can make the changes.

@julienrbrt
Copy link
Member

@julienrbrt you fine with merging this and then i can make the changes.

Works! We need changelogs as well to reflect some API breaks from here in the follow-up

@tac0turtle tac0turtle merged commit b42a0bc into cosmos:master Aug 11, 2023
7 checks passed
@julienrbrt
Copy link
Member

@Mergifyio backport release/1.x.x

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2023

backport release/1.x.x

❌ No backport have been created

  • Backport to branch release/1.x.x failed

GitHub error: Branch not found

@julienrbrt
Copy link
Member

@Mergifyio backport release/v1.x.x

@tac0turtle
Copy link
Member

@Mergifyio backport release/v1.x.x

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2023 •
edited
Loading

backport release/v1.x.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 11, 2023
@catShaark @mergify
feat: Uses BatchWithFlusher in iavl tree (#807)
bf4c9f3 
(cherry picked from commit b42a0bc)
@mergify mergify bot mentioned this pull request Aug 11, 2023
Merged
tac0turtle pushed a commit that referenced this pull request Aug 11, 2023
@mergify @catShaark
feat: Uses BatchWithFlusher in iavl tree (backport #807) (#816)
ec9839f 
Co-authored-by: khanh-notional <[email protected]>
@tac0turtle tac0turtle mentioned this pull request Aug 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cool-develope cool-develope cool-develope approved these changes

@tac0turtle tac0turtle tac0turtle approved these changes

Assignees

@cool-develope cool-develope

Labels
backport/v1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@catShaark @julienrbrt @tac0turtle @cool-develope