chore: cleanup unnecessary `ndb.Commit` calls #902

cool-develope · 2024-03-07T13:58:03Z

Context

There are some unnecessary ndb.Commit and resetBatch calls which introduced to avoid the large batch commit originally. They are not necessary since we are using BatchWithFlusher now.

coderabbitai · 2024-03-07T13:58:10Z

Walkthrough

The recent modifications streamline the codebase by simplifying commit processes and batch operation logic. The changes remove unnecessary variables and parameters such as commitGap and isGenesis, and eliminate batch-related functions, thus enhancing code clarity and reducing complexity. These adjustments aim to improve the efficiency of node saving and version handling, particularly in the context of fast node additions and database operations, making the system more straightforward and less error-prone.

Changes

Files	Change Summary
`mutable_tree.go`	Removed `commitGap` and associated logic, streamlined `SaveVersion` by removing `isGenesis`, simplified `saveFastNodeAdditions`.
`mutable_tree_test.go`	Adjusted test expectations for database operations, batch writes, and closures, updated test cases for storage upgrade.
`nodedb.go`	Removed `resetBatch` and `writeBatch` functions, simplified `SaveNode` method by eliminating batch operation conditions.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

ValarDragon

Awesome! Just need to get cosmos-db sorted out for batch with flusher on osmosis

tac0turtle · 2024-03-07T22:35:04Z

backport?

cool-develope · 2024-03-13T13:16:19Z

backport?

not sure, @czarcas7ic osmosis implemented the db wrapper with flusher fully?
I think we can merge it, and later backport when need

czarcas7ic · 2024-03-20T03:02:32Z

@cool-develope I didn't understand how to implement in v0.37, see https://osmosis-network.slack.com/archives/C04LXMASL0P/p1708037594466689?thread_ts=1707244984.568649&channel=C04LXMASL0P&message_ts=1708037594.466689

czarcas7ic · 2024-03-22T01:36:59Z

Thanks for implementing this for our sdk fork @cool-develope! Seems we are unblocked to merge so we can test

czarcas7ic · 2024-03-22T02:27:08Z

Sorry maybe I am not fully understanding how implementing with flusher has to do with this PR?

In the PR you made @cool-develope, it seems like NewBatch and NewBatchWithSize still behave the same, which I thought that was what needed to be addressed https://github.com/osmosis-labs/cosmos-sdk/blob/6c1422877673277448bc14e2be415133cc6738e6/store/wrapper/tmdb.go#L52-L57

My apologies if my understanding is off here.

cool-develope · 2024-03-22T12:16:04Z

Sorry maybe I am not fully understanding how implementing with flusher has to do with this PR?

In the PR you made @cool-develope, it seems like NewBatch and NewBatchWithSize still behave the same, which I thought that was what needed to be addressed https://github.com/osmosis-labs/cosmos-sdk/blob/6c1422877673277448bc14e2be415133cc6738e6/store/wrapper/tmdb.go#L52-L57

My apologies if my understanding is off here.

NewBatchSize is aimed to allocate the memory when create the batch, it will allow to avoid memory grow while batch insertion. It is not directly related to the flusher. The purpose of flusher is to avoid the large batch writing, it writes periodically if the batch size is greater than the threshold.
Ofc, NewBatchSize will increase the performance, unfortunately I have no idea of how to refactor the prefixdb of comet-db. However, we can merge it without NewBatchSize still.
You don't need to refactor anything for this PR since nodeDB internally use BatchWithFlusher, just merge it (osmosis-labs/cosmos-sdk#582) and update the iavldb with this commit, then check if the batch writing is triggered periodically.

czarcas7ic · 2024-03-22T16:03:04Z

Thanks for the explanation!

cool-develope · 2024-03-22T19:45:59Z

@Mergifyio backport release/v1.1.x

mergify · 2024-03-22T19:46:06Z

backport release/v1.1.x

✅ Backports have been created

#921 chore: cleanup unnecessary ndb.Commit calls (backport #902) has been created for branch release/v1.1.x

Co-authored-by: Adam Tucker <[email protected]> (cherry picked from commit 5a9cc69)

Co-authored-by: cool-developer <[email protected]>

cleanup unnecessary commit

cd8eed0

cool-develope requested a review from a team as a code owner March 7, 2024 13:58

ValarDragon approved these changes Mar 7, 2024

View reviewed changes

tac0turtle approved these changes Mar 7, 2024

View reviewed changes

Merge branch 'master' into chore/wire_flush

591c111

cool-develope merged commit 5a9cc69 into master Mar 22, 2024
7 checks passed

cool-develope deleted the chore/wire_flush branch March 22, 2024 19:44

mergify bot pushed a commit that referenced this pull request Mar 22, 2024

chore: cleanup unnecessary ndb.Commit calls (#902)

8a38162

Co-authored-by: Adam Tucker <[email protected]> (cherry picked from commit 5a9cc69)

mergify bot mentioned this pull request Mar 22, 2024

chore: cleanup unnecessary ndb.Commit calls (backport #902) #921

Merged

cool-develope added a commit that referenced this pull request Mar 25, 2024

chore: cleanup unnecessary ndb.Commit calls (backport #902) (#921)

ff86011

Co-authored-by: cool-developer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: cleanup unnecessary `ndb.Commit` calls #902

chore: cleanup unnecessary `ndb.Commit` calls #902

cool-develope commented Mar 7, 2024

coderabbitai bot commented Mar 7, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

ValarDragon left a comment

tac0turtle commented Mar 7, 2024

cool-develope commented Mar 13, 2024

czarcas7ic commented Mar 20, 2024 •

edited

Loading

czarcas7ic commented Mar 22, 2024

czarcas7ic commented Mar 22, 2024

cool-develope commented Mar 22, 2024 •

edited

Loading

czarcas7ic commented Mar 22, 2024

cool-develope commented Mar 22, 2024

mergify bot commented Mar 22, 2024 •

edited

Loading

chore: cleanup unnecessary ndb.Commit calls #902

chore: cleanup unnecessary ndb.Commit calls #902

Conversation

cool-develope commented Mar 7, 2024

Context

coderabbitai bot commented Mar 7, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

ValarDragon left a comment

Choose a reason for hiding this comment

tac0turtle commented Mar 7, 2024

cool-develope commented Mar 13, 2024

czarcas7ic commented Mar 20, 2024 • edited Loading

czarcas7ic commented Mar 22, 2024

czarcas7ic commented Mar 22, 2024

cool-develope commented Mar 22, 2024 • edited Loading

czarcas7ic commented Mar 22, 2024

cool-develope commented Mar 22, 2024

mergify bot commented Mar 22, 2024 • edited Loading

✅ Backports have been created

chore: cleanup unnecessary `ndb.Commit` calls #902

chore: cleanup unnecessary `ndb.Commit` calls #902

coderabbitai bot commented Mar 7, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

czarcas7ic commented Mar 20, 2024 •

edited

Loading

cool-develope commented Mar 22, 2024 •

edited

Loading

mergify bot commented Mar 22, 2024 •

edited

Loading