feat: ADR for api cleanup and async commit #790
Conversation
| The modified API will have the following structure: | ||
|
|
||
| ```go | ||
| Tree interface { |
There was a problem hiding this comment.
imo having read-only views as an interface makes sense
There was a problem hiding this comment.
I am just thinking iavl will be used as a SC layer and it may mislead if have a read-only interface.
Also, we have only one interface for iavl tree in store module.
There was a problem hiding this comment.
read only would be good as we still need the iavl tree for commitments, store v2 adr talks close to nothing about this. I wouldn't use the current adr as final its very very high level with a lack of experimentation and research
|
I'd generally still suggest starting with separating out version management (#737 ) |
@tac0turtle @yihuang , how about separating out version manager? |
I'd suggest figure out the minimal changes needed to support store v2 first (async commit, atomic commit), I don't see a big problem in current API design. |
|
lets separate concerns, first what is needed for store v2 then a new api. I think conflating the two could be clouding what is all needed |
|
Do we really require the |
| VersionExists(version int64) bool | ||
| DeleteVersionsTo(version int64) error | ||
| GetVersioned(key []byte, version int64) ([]byte, *cmtprotocrypto.ProofOps, error) | ||
| GetTree(version int64) (*iavl.Tree, error) |
There was a problem hiding this comment.
It could be unnecessary if we move all query functionalities to SS...
There was a problem hiding this comment.
Awesome work @cool-develope! I think this is a good jumping off point.
I can imagine, however, some of the finer points needing amendment as Store v2 work kicks off.
|
im still a bit lost with this adr, API cleanup has nothing to do with store v2. The commitment structure should be agnostic to what is going on at a higher level. I think we should remove any notion of store v2 and the plans there and work on creating a stable and sane API for this code base |
cool-develope
changed the title
ok, I modified the ADR, and assumed |
tac0turtle
changed the title
| - The boundary between `ImmutableTree` and `MutableTree` is not clear. | ||
| - There are many public methods in `nodeDB`, making it less structured. | ||
|
|
||
| ## Decision |
There was a problem hiding this comment.
what is the external api to users? the below seems to focus on internal changes.
There was a problem hiding this comment.
do you think iavl needs the external API interface to clarify? I plan to keep the majority external API
|
just removed the parallel WorkingHash part, I think it is unnecessary since we can parallelize on a module basis. |
| ### Async Commit | ||
|
|
||
| * Finalize the current version in memory during `Commit`. | ||
| * Perform async writes in the background. |
There was a problem hiding this comment.
what are the side effects to this?
|
|
||
| ```go | ||
| MutableTree interface { | ||
| SaveChangeSet(cs *ChangeSet) ([]byte, error) // this is for batch set |
There was a problem hiding this comment.
similar to batch, just wanted to re-use the ChangeSet which is already defined in the iavl
|
|
||
| ### Negative | ||
|
|
||
| * Async Commit may result in increased memory usage and increased code complexity. |
There was a problem hiding this comment.
i would add the added complexity in the code base. the asyncommit adds a layer of complexity
There was a problem hiding this comment.
do you want to introduce some pseudo-code within this adr?
There was a problem hiding this comment.
it can be added to a con, no need for pseudo-code
There was a problem hiding this comment.
sorry, a little confusing
do you want to add a more detailed description in increased code complexity?
|
it is not iavl 2.0, just simple API cleanup |
No description provided.