fix: safe batch write - Githubissues

cool-develope commented 1 year ago

Context

We faced with value missing error sometimes. I think it is related to recent works of nodekey refactoring and batch flusher.

Before batch flusher, we have one atomic batch write but now there can be several batch writes, so some batch data can be away when the app is in panic.
Originally, we stored the root in the last order but now it is stored at first after nodekey refactoring

As a result, there may be a situation that has only a root node, not entire nodes storing.

Action

Refactored the storing nodes order to save the root at last

Summary by CodeRabbit

Refactor: Updated the handling of 'fast storage' in our system to improve code clarity and efficiency. This includes changes to how we save new nodes and manage versions in our 'MutableTree' and 'nodeDB' structures.
Test: Adjusted several test cases to align with the updated code structure and behavior. This includes changes to 'TestLegacyReferenceNode', 'TestNodeCacheStatisic', and several tests in 'nodedb_test.go'.

These changes are primarily under-the-hood improvements and should not affect the user experience directly. However, they lay the groundwork for more efficient data handling and future feature development.

tac0turtle commented 1 year ago

were you able to test the issue berachain ran into here?

kocubinski commented 1 year ago

It should be possible to prove the theory in this PR as to the cause of value missing error. Can you create a failing test case which is then fixed by this patch?

cool-develope commented 1 year ago

It should be possible to prove the theory in this PR as to the cause of value missing error. Can you create a failing test case which is then fixed by this patch?

The main reason of this issue is due to batch writing breaks, I have no idea of how to break the batch writing in test. To simulate the above issue, we should break the batch writing while commit, and re-load the tree. Do you have any idea?

cool-develope commented 12 months ago

@elias-orijtech @odeke-em This is a PR that was mentioned before. I struggle to add test cases since the issue concerns low-level API (batch writing). The failure case is triggered by loading the broken tree. To break the tree state, the app should panic while writing the batch.

elias-orijtech commented 12 months ago

@cool-develope I don't have any comments apart from what @kocubinski said about needing a test case to make sure this PR fixes what it claims.

cool-develope commented 12 months ago

@cool-develope I don't have any comments apart from what @kocubinski said about needing a test case to make sure this PR fixes what it claims.

That's what I struggle now, could you help me add a test case?

odeke-em commented 12 months ago

Thanks @cool-develope and @tac0turtle for the ping, I am working on helping out with the test for this.

kocubinski commented 11 months ago

Admittedly, it's a bit hard to provide a thorough test case give we'd need to work with the internals of go leveldb batching. I think I'm OK merging this as-is; moving the creation of the batch from pre-order to post-order should move the entire SaveVersion batch to behave like a transaction.

coderabbitai[bot] commented 11 months ago

Walkthrough

The changes primarily focus on improving the handling of the fast storage version in the MutableTree and nodeDB structures. The latest version is now passed as an argument to relevant methods, enhancing code clarity and decoupling. Test cases have been updated to reflect these changes.

Changes

File(s)	Summary
`migrate_test.go`	Updated `legacyVersion` variable in `TestLegacyReferenceNode` function.
`mutable_tree.go`	Enhanced handling of fast storage version in `MutableTree` struct and its methods.
`nodedb.go`, `nodedb_test.go`	Refactored `setFastStorageVersionToBatch` function in `nodeDB` struct to take `latestVersion` as an argument. Updated test cases accordingly.
`tree_test.go`	Updated expected values in `TestNodeCacheStatisic` test case.

🐇💻

In the land of code, where logic is king,

Changes were made, improvements they bring.

Fast storage version, handled with care,

Tests updated too, fair and square.

Celebrate the changes, for they are fine,

In the world of code, where we rabbits shine. 🎉🥕

Tips

### Chat with CodeRabbit Bot (`@coderabbitai`) - Mention `@coderabbitai` in any *review comment* for bot assistance. - Note: Review comments are made on code diffs or files, not on the PR overview. ### Pause Incremental Reviews - Insert `@coderabbitai: ignore` in the PR description to halt the bot's ongoing reviews. Remove the line to resume. - Additionally, enter `@coderabbitai pause` to halt the bot's ongoing reviews as any issue or review comment. To resume reviews, enter `@coderabbitai resume` as any issue or review comment.

tac0turtle commented 11 months ago

@mergifyio backport release/v1.x.x

mergify[bot] commented 11 months ago

backport release/v1.x.x

✅ Backports have been created

* [#845 fix: safe batch write (backport #838)](https://github.com/cosmos/iavl/pull/845) has been created for branch `release/v1.x.x`

cosmos / iavl

fix: safe batch write #838