Skip to content

feat: Uses BatchWithFlusher in iavl tree#807

Merged
tac0turtle merged 4 commits intocosmos:masterfrom
notional-labs:batchflushing
Aug 11, 2023
Merged

feat: Uses BatchWithFlusher in iavl tree#807
tac0turtle merged 4 commits intocosmos:masterfrom
notional-labs:batchflushing

Conversation

@catShaark
Copy link
Contributor

@catShaark catShaark commented Aug 8, 2023

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 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
Contributor

@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
Contributor

@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
Contributor

julienrbrt commented Aug 10, 2023

+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
Contributor

tac0turtle commented Aug 11, 2023

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

@julienrbrt
Copy link
Contributor

julienrbrt commented Aug 11, 2023

@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
@julienrbrt
Copy link
Contributor

julienrbrt commented Aug 11, 2023

@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

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

GitHub error: Branch not found

@julienrbrt
Copy link
Contributor

julienrbrt commented Aug 11, 2023

@Mergifyio backport release/v1.x.x

@tac0turtle
Copy link
Contributor

tac0turtle commented Aug 11, 2023

@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

Details

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 <50263489+catShaark@users.noreply.github.com>
@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

2 more reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

@cool-develope cool-develope

Labels

backport/v1.x

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@catShaark @julienrbrt @tac0turtle @cool-develope