Use Datasource interface #620

Activity

1. tzdybal

   added this to Execution Environmentson Nov 21, 2022
2. tzdybal

   moved this to TODO in Execution Environmentson Nov 21, 2022
3. tzdybal

   assigned

   gupadhyaya

   on Nov 23, 2022
4. gupadhyaya commented on Dec 1, 2022

   

   gupadhyaya

   on Dec 1, 2022

   Contributor

   More actions

   Context
   Rollmint's datastore is based on a simple type KVStore interface { with type Batch interface { and type Iterator interface { functionalities and also supports substores via type PrefixKV struct {. The underlying implementation of KVStore is type BadgerKV struct { which uses github.com/dgraph-io/badger/v3.

   Problem
   The usecases like connection gating (#610) requires that the datastore used in the rollmint node is datastore.Datastore interface compatible. Right now it is only KVStore interface compatible. We were exploring following possible solutions.

   Solutions

   1. replace current interface with Datastore in rollmint codebase
   2. implement Datastore interface using rollmint code
   3. implement rollmint interfaces using Datastore interface

   Analysis

   • while the datastore.Datastore provides ETL functionalities with batch and substores (in the form of keytransform), it doesn't directly provide prefix iteration methods. So, if we use solution 1, we will end up creating an extended interface anyways with additional functionalities for prefix iteration.
   • implementing datastore interface using rollmint code will be similar to what go-ds-badger3 is doing, we will basically repeat the efforts. however, note that we cannot directly replace our KVStore implementation, which is BadgerKV with go-ds-badger3 as the datastore interface methods require parameters like context and key in the form of datastore.Key not plain []byte. e.g., func (d *Datastore) Get(ctx context.Context, key ds.Key). doing both datastore.Datastore methods and KVStore methods will add unnecessary repetitive code. for example: two Get functions like:

   func (d *Datastore) Get(ctx context.Context, key ds.Key) (value []byte, err error) {
   func (b *BadgerKV) Get(key []byte) ([]byte, error) {
   

   instead what we should do is, adapt our KVStore interface to be datastore.Datastore compatible, for example, use datastore.Key instead of []byte for keys, use context, etc, such that, we can easily swap out the underlying BadgerKV implementation with go-ds-badger3. after swapping out, we may need to extend the implementation with some additional prefix iteration capabilities, which should be feasible. This way, we will have minimal changes to KVStore consumer, yet the KVStore implementation will also be datastore.Datastore interface compatible and hence node's datastore (KVStore) can now be used for usecases like connection gating.

   • third solution of implementing rollmint interfaces using datastore does not really solve our problem, because the resulting solution won't be datastore.Datastore interface compatible and cannot be consumed by connection gating usecase.

   Summary
   I propose:

   • adapting KVStore to be datastore.Datastore compatible
   • use go-ds-badger3 implementation (which is already datastore.Datastore interface compatible)
   • add missing prefix iteration methods to the go-ds-badger3's datastore by type aliasing and embeded struct

   @tzdybal @nashqueue @Manav-Aggarwal

   👀React with 👀1Reacted by Wondertan
5. tzdybal commented on Dec 1, 2022

   

   tzdybal

   on Dec 1, 2022

   ContributorAuthor

   More actions

   It's cool that go-ds-badger3 is out! When I was creating issue, it was not there 😁

   Can we use Query to get prefix iteration? go-ds-badger3 implementation shows it internally use badger Prefix iterator option.
   https://github.com/ipfs/go-ds-badger3/blob/666814959fdac3acd0398e67cc1821aceddfb92b/datastore.go#L736
6. gupadhyaya commented on Dec 1, 2022

   

   gupadhyaya

   on Dec 1, 2022

   Contributor

   More actions

   It's cool that go-ds-badger3 is out! When I was creating issue, it was not there 😁

   Can we use Query to get prefix iteration? go-ds-badger3 implementation shows it internally use badger Prefix iterator option. https://github.com/ipfs/go-ds-badger3/blob/666814959fdac3acd0398e67cc1821aceddfb92b/datastore.go#L736

   should be possible, however the client side needs to be adapted to work on the iteration result than iterating itself. will look more closely, but should be more of an impl nuance.
7. Wondertan commented on Dec 1, 2022

   

   Wondertan

   on Dec 1, 2022

   Contributor

   More actions

   Good analysis @gupadhyaya. Adding my 2 cents about 'future proofness'. Having two KVStore abstractions will create maintenance issues and ambiguity within the codebase. Long-term we might aim for one ksstore interface, even if that would require us to migrate existing code which can be done incrementally.
8. tzdybal

   moved this from TODO to In Progress in Execution Environmentson Dec 1, 2022
9. gupadhyaya commented on Dec 2, 2022

   

   gupadhyaya

   on Dec 2, 2022

   Contributor

   More actions

   Good analysis @gupadhyaya. Adding my 2 cents about 'future proofness'. Having two KVStore abstractions will create maintenance issues and ambiguity within the codebase. Long-term we might aim for one ksstore interface, even if that would require us to migrate existing code which can be done incrementally.

   good point. i think we can fully switch over to datastore with some careful migration of all client code. what do u say @tzdybal ?

   👍React with 👍2Reacted by nashqueue and Wondertan
10. tzdybal commented on Dec 2, 2022

    

    tzdybal

    on Dec 2, 2022

    ContributorAuthor

    More actions

    I totally agree 👍
11. tzdybal

    moved this from In Progress to In review in Execution Environmentson Jan 2, 2023
12. gupadhyaya

    closed this as completedin #665on Jan 9, 2023
13. github-project-automation

    moved this from In review to Done in Execution Environmentson Jan 9, 2023