Closed cool-develope closed 3 months ago
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.
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?
backport?
backport?
not sure, @czarcas7ic osmosis implemented the db wrapper with flusher fully? I think we can merge it, and later backport when need
@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
Thanks for implementing this for our sdk fork @cool-develope! Seems we are unblocked to merge so we can test
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.
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 (https://github.com/osmosis-labs/cosmos-sdk/pull/582) and update the iavldb with this commit, then check if the batch writing is triggered periodically.
Thanks for the explanation!
@Mergifyio backport release/v1.1.x
backport release/v1.1.x
Context
There are some unnecessary
ndb.Commit
andresetBatch
calls which introduced to avoid the large batch commit originally. They are not necessary since we are usingBatchWithFlusher
now.