Firewoodize merkle db Part 1: Make Views ReadOnly #1816
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dboehm-avalabs
requested review from
danlaine,
darioush,
abi87,
gyuho,
joshua-kim,
hexfusion,
StephenButtolph,
coffeeavax and
ceyonur
as code owners
darioush
reviewed
Aug 7, 2023
danlaine
approved these changes
Aug 8, 2023
There was a problem hiding this comment.
Looks OK to me but two things I want to flag:
- We still need to update the readme
- The code is in a kind of intermediate state since we've made views read only, but still have a lot of code (mostly locking) that helped to handle views being editable. Should we wait until subsequent PRs cleaning up that code have been written/merged into this one before merging this into dev?
Comment on lines
-56
to
-66
| // Remove will delete a key from the Trie | ||
| Remove(ctx context.Context, key []byte) error | ||
|
|
||
| // NewPreallocatedView returns a new view on top of this Trie with space allocated for changes | ||
| NewPreallocatedView(estimatedChanges int) (TrieView, error) | ||
|
|
||
| // NewView returns a new view on top of this Trie | ||
| NewView() (TrieView, error) | ||
|
|
||
| // Insert a key/value pair into the Trie | ||
| Insert(ctx context.Context, key, value []byte) error |
I think we risk having an enormous, unreviewable mess if we try to merge everything at once. I think iterative cleanup might be best. |
darioush
approved these changes
Aug 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove the ability to insert/remove key values from merkledb directly. Can make changes by passing them in as a batch when creating a new view.