Skip to content

migrate datasource from store.KVStore to datastore.Datastore - #665

Merged
gupadhyaya merged 10 commits into
evstack:mainfrom
gupadhyaya:datasource_migration
Jan 9, 2023
Merged

migrate datasource from store.KVStore to datastore.Datastore#665
gupadhyaya merged 10 commits into
evstack:mainfrom
gupadhyaya:datasource_migration

Conversation

@gupadhyaya

@gupadhyayagupadhyaya commented Dec 15, 2022

Copy link
Copy Markdown
Contributor

Fixes#620

description of the major change:

  • prefix changes from byte array to strings, e.g, []byte{0} to "0". this is required as datastore Key uses strings and does not support byte array. the changes are done for {mainPrefix, dalcPrefix, indexerPrefix} also for {blockPrefix, indexPrefix, commitPrefix, statePrefix, responsesPrefix, validatorsPrefix}.
  • some examples of storage keys after this change are:
    • blockhash: "/0/b/<block-hash>"
    • block header: "/0/i/<block-header-num>"
    • transaction indexer: "/2/tx.height/4/4/1"
    • block indexer: "/2/block_events/block.height/10"

review can follow this path:

  • start with node.go
  • which leads to 1) main storage (store.go), 2) dalc (files under da/ package), 3) indexer storage (block indexer: state/indexer/block/kv/kv.go and tx indexer: state/txindex/kv/kv.go)
  • that's it, rest of the files are minor changes

@gupadhyayagupadhyaya changed the title migrate datasource from store.KVStore to datastore.Datastore in go-datastore[wip] migrate datasource from store.KVStore to datastore.Datastore in go-datastoreDec 15, 2022
@gupadhyayagupadhyaya changed the title [wip] migrate datasource from store.KVStore to datastore.Datastore in go-datastoremigrate datasource from store.KVStore to datastore.Datastore in go-datastoreDec 21, 2022
@gupadhyayagupadhyaya changed the title migrate datasource from store.KVStore to datastore.Datastore in go-datastoremigrate datasource from store.KVStore to datastore.DatastoreDec 21, 2022
@codecov-commenter

codecov-commenter commented Dec 21, 2022

Copy link
Copy Markdown

Codecov Report

Merging #665 (fd1864f) into main (de83cbc) will decrease coverage by 0.42%.
The diff coverage is 76.81%.

@@ Coverage Diff @@## main #665 +/- ##
==========================================
- Coverage 55.49% 55.06% -0.43% 
==========================================
Files 50 48 -2 Lines 10353 10206 -147 ==========================================
- Hits 5745 5620 -125 + Misses 3739 3724 -15 + Partials 869 862 -7 
Impacted FilesCoverage Δ
mempool/cache.go84.21% <ø> (ø)
mempool/clist/clist.go75.00% <ø> (ø)
mempool/mempool.go52.77% <ø> (ø)
state/txindex/indexer_service.go62.96% <ø> (ø)
da/celestia/mock/server.go49.68% <40.00%> (-0.64%)⬇️
state/indexer/block/kv/kv.go63.08% <68.08%> (+4.19%)⬆️
state/txindex/kv/kv.go64.56% <72.41%> (-2.11%)⬇️
node/node.go60.43% <76.47%> (-0.28%)⬇️
da/mock/mock.go73.75% <76.92%> (-5.80%)⬇️
store/store.go61.39% <84.84%> (-2.36%)⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment threadda/da.go

@tzdybaltzdybal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! But I still have some questions and comments:

  1. We probably should use one of interfaces defined in go-datastore, instead of concrete go-ds-badger3 type in most places. For example celestia-node is using ds.Batching instead of ds.Datastore.
  2. I don't get the difference between Batching and TxnDatastore / Batch and Txn - probably we should investigate this a bit more.
  3. I think that we should return query.Results to avoid instantiating all the result objects at once.

Comment threadda/celestia/celestia.go Outdated
Comment threadstate/indexer/block/kv/kv.go Outdated
Comment threadstore/kv.go Outdated
Comment threadda/mock/mock.go Outdated
Comment threadda/da.go

@nashqueuenashqueue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look great. Most of my comments are nits and optional. Giving me the path to review helped a lot in understanding the bigger picture. Thank you :)

Comment threadstore/store.go Outdated
Comment threadstore/store.go
Comment threadstore/store.go
Comment threadnode/node.go
Comment threadstate/indexer/block/kv/kv.go
Comment threadstate/indexer/block/kv/util.go

@nashqueuenashqueue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tzdybaltzdybal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@gupadhyaya
gupadhyaya merged commit 90ff206 into evstack:mainJan 9, 2023
tzdybal pushed a commit that referenced this pull request Jan 13, 2023
* fixing merge conflicts
* fix breaking changes related to substore, prefixing, and keys
* fix all the failing tests
* test that is no longer needed
* fix missing merge conflict
* fixing golangci-lint errors
* more lint errors
* goimports fixes
* fixing golangci-lint failures
* changing from DataStore to TxnDatasource, cutting some LOC, prefix entires as iterator, etc
Co-authored-by: Ganesha Upadhyaya <gupadhyaya@Ganeshas-MacBook-Pro-2.local>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Datasource interface

4 participants

@gupadhyaya@codecov-commenter@tzdybal@nashqueue