Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pkg/db): remove cleveldb, fsdb and rocksdb #1714

Merged
merged 9 commits into from
May 15, 2024

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Mar 1, 2024

Split from #1602. Context:

@zivkovicmilos:1 Unpopular opinion: we should probably just drop these DBs entirely from the codebase, they are never gonna be used in place of LevelDB in the near future

@ajnavarro: IMHO the needed database is defined by the application needs, not from any other external usecases. Sticking to one of the existing key/value databases, we can use their special capabilities to make it more performant for our use case (yes, even if they are all key/value DBs, they differ in a lot of low-level functionality).
I would say to use one and add support in the future for more if needed.

After a discussion in the review meeting w/ @jaekwon, this PR is now changed to maintain another reference implementation with boltdb. It also makes sure that it is selectable, when available, from the tm2 node configuration.

After this PR, there are no databases in the codebase using cgo.

Footnotes

  1. https://github.com/gnolang/gno/pull/1602#discussion_r1490862277

@thehowl thehowl self-assigned this Mar 1, 2024
@thehowl thehowl requested review from moul, a team, gfanton, jaekwon, piux2 and zivkovicmilos as code owners March 1, 2024 12:13
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Mar 1, 2024
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.20%. Comparing base (8afb1a4) to head (c3f91b7).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1714      +/-   ##
==========================================
+ Coverage   47.00%   47.20%   +0.19%     
==========================================
  Files         578      575       -3     
  Lines       77582    77092     -490     
==========================================
- Hits        36471    36393      -78     
+ Misses      38076    37666     -410     
+ Partials     3035     3033       -2     
Flag Coverage Δ
contribs/gnodev 24.14% <ø> (ø)
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 61.68% <ø> (ø)
gnovm 35.97% <ø> (-0.02%) ⬇️
misc/autocounterd 0.00% <ø> (ø)
misc/genproto 0.00% <ø> (ø)
misc/genstd 73.90% <ø> (ø)
misc/goscan 0.00% <ø> (ø)
misc/logos 17.68% <ø> (+0.30%) ⬆️
misc/loop 0.00% <ø> (ø)
tm2 54.58% <100.00%> (+0.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tm2/pkg/db/goleveldb/go_level_db_test.go Outdated Show resolved Hide resolved
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

3nkh17

tm2/pkg/db/goleveldb/go_level_db_test.go Outdated Show resolved Hide resolved
@thehowl
Copy link
Member Author

thehowl commented Mar 28, 2024

From the review meeting: let's keep one additional implementation aside from the ones that are currently used.

For what @ajnavarro was referring to, we can support functionality of specific databases by adding extra methods, and then using type assertions in usage to verify if they support that (this would likely have to be done anyway, even if we keep as it is now only goleveldb and memdb.)

I will update the PR and add support boltdb

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label May 6, 2024
@thehowl thehowl changed the title feat(pkg/db): remove all dbs except for goleveldb and memdb feat(pkg/db): remove cleveldb, fsdb and rocksdb May 6, 2024
@thehowl
Copy link
Member Author

thehowl commented May 6, 2024

This PR is now updated, and ready for a last review before merging.

@thehowl thehowl merged commit a3a9b56 into master May 15, 2024
192 checks passed
@thehowl thehowl deleted the dev/morgan/remove-unused-dbs branch May 15, 2024 09:34
leohhhn pushed a commit to leohhhn/gno that referenced this pull request May 21, 2024
Split from gnolang#1602. Context:

> @zivkovicmilos:[^1] Unpopular opinion: we should probably just drop
these DBs entirely from the codebase, they are never gonna be used in
place of LevelDB in the near future
>
> @ajnavarro: IMHO the needed database is defined by the application
needs, not from any other external usecases. Sticking to one of the
existing key/value databases, we can use their special capabilities to
make it more performant for our use case (yes, even if they are all
key/value DBs, they differ in a lot of low-level functionality). \
> I would say to use one and add support in the future for more if
needed.

After a discussion in the review meeting w/ @jaekwon, this PR is now
changed to maintain another reference implementation with boltdb. It
also makes sure that it is selectable, when available, from the tm2 node
configuration.

After this PR, there are no databases in the codebase using cgo.

[^1]: gnolang#1602 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants