cometbft / cometbft-db

Database wrapper for CometBFT
Apache License 2.0
29 stars 51 forks source link

remove Dockerfile, use only pure go dabase backends #135

Closed faddat closed 7 months ago

faddat commented 8 months ago

This PR:

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.26%. Comparing base (fb4f703) to head (e3118b3). Report is 11 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135/graphs/tree.svg?width=650&height=150&src=pr&token=QcxCp5s2sq&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft)](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) ```diff @@ Coverage Diff @@ ## main #135 +/- ## ========================================== + Coverage 76.80% 77.26% +0.45% ========================================== Files 23 14 -9 Lines 2048 1060 -988 ========================================== - Hits 1573 819 -754 + Misses 403 193 -210 + Partials 72 48 -24 ``` | [Files](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) | Coverage Δ | | |---|---|---| | [pebble.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-cGViYmxlLmdv) | `72.64% <100.00%> (-0.92%)` | :arrow_down: | | [badger\_db.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-YmFkZ2VyX2RiLmdv) | `89.03% <33.33%> (+1.25%)` | :arrow_up: | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) | [Files](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) | Coverage Δ | | |---|---|---| | [pebble.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-cGViYmxlLmdv) | `72.64% <100.00%> (-0.92%)` | :arrow_down: | | [badger\_db.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-YmFkZ2VyX2RiLmdv) | `89.03% <33.33%> (+1.25%)` | :arrow_up: | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/cometbft/cometbft-db/pull/135/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft)
faddat commented 8 months ago

@melekes -- this is my new proposed PR. It is simpler, easier to maintain, and faster all around.

It will also reduce dramatically the amount of work needed to maintain both this repository and cometbft.

The correct answer to your question about the Dockerfile, is that there's been no reason to have a Dockerfile in this repository for years.

faddat commented 8 months ago

When this PR started, tests took more than ten minutes. They now take less than one minute.

melekes commented 8 months ago

Why keep goleveldb if it's unmaintained?

faddat commented 8 months ago

@melekes -- backwards compatibility for in production chains.

Unfortunately I think we will want to support it for some time to come.

The best solution would be to make sure that no new chains are started using goleveldb, and include in the notes on db config that teams should migrate from goleveldb to pebble.

github-actions[bot] commented 8 months ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

faddat commented 8 months ago

@melekes - this is now being marked as stale, do you reckon we want to use it?

melekes commented 8 months ago

I agree with the general direction 👍 and would like to see this merged.

faddat commented 7 months ago

Cool, please let me know what else can be polished up to get this in.

faddat commented 7 months ago

I think I just fixed that annoying codeql thing.

melekes commented 7 months ago

eliminates unmaintained database backends with no future: bolt badger

badger is very much maintained https://github.com/dgraph-io/badger

faddat commented 7 months ago

@melekes it is pure go, too, so would you like to keep it?

faddat commented 7 months ago

I think this is ready to rock now, but there is a valid question of weather or not to include badger.

Badger v4 worked out of the box. No valid reason not to include it.

faddat commented 7 months ago

If we are going to merge this, we should. Blocks upgrading comet to go 1.22

adizere commented 7 months ago

I would prefer to sunset database backends iteratively:

  1. mark them as deprecated -- comet-db v0.12
    • then give it some time for users to acclimatize
  2. remove them -- comet-db v0.13

I would also keep rocksdb around, since some users (at least Cronos, potentially others) clearly make use of it.

Therefore, we should separate this PR into multiple smaller PRs:

@faddat do you plan on helping with any of the above? if so, please open corresponding PRs. if not, we plan to work on it this week so we'll go ahead within a couple of days

faddat commented 7 months ago

@adizere

it is necessary to update the dockerfile in order to update grocksdb. So ci will take a very long time now.

As for phases, that's fine.

We will not be able to improve docker without removing rocksdb (and other databases that use cgo)