cosmos / iavl

Merkleized IAVL+ Tree implementation in Go
Apache License 2.0
413 stars 256 forks source link

refactor: make batcher configurable #815

Closed tac0turtle closed 11 months ago

tac0turtle commented 11 months ago

follow up to https://github.com/cosmos/iavl/pull/807

julienrbrt commented 11 months ago

The issue I have with simply adding an option and still taking the whole struct, is that you may overwrite a good default option that you haven't provided.

Example here: https://github.com/cosmos/cosmos-sdk/blob/66d9be0e1080ab6c7e013fc83bfed18ddd1b3c14/store/iavl/store.go#L54

We could simplify the api by adding a variadic way to update the options. Bonus is that NewMutableTreeWithOpts can still be used to provide all options, so there is no API break (or we clean it up and remove it).

type Option func(*Options)
NewMutableTree(db dbm.DB, cacheSize int, skipFastStorageUpgrade bool, lg log.Logger, options ...Option) *MutableTree {
    opts := DefaultOptions()
    for _, opt := range options {
        opt(&opts)
    }

    return NewMutableTreeWithOpts(...)
}

Unless we are ok with that risk, and then it lgtm!

cool-develope commented 11 months ago

I like @julienrbrt 's idea, we should make sure to set the default options even if users miss the config

cool-develope commented 11 months ago

testing failed ?

julienrbrt 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

* [#818 refactor: make batcher configurable (backport #815)](https://github.com/cosmos/iavl/pull/818) has been created for branch `release/v1.x.x` but encountered conflicts