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

remove Dockerfile, use only pure go dabase backends #135

Closed
wants to merge 42 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Feb 15, 2024

This PR:

  • updates golang to v1.22.* and pebble v1.1.0
  • allows cometbft downstream to reduce its usage of Docker
  • reduces CI time on this repository by over 95%
  • keeps goleveldb for backwards compatiblity, and badger because it's pure go
  • eliminates unmaintained database backends with no future:
    • bolt
  • eliminates database backends that complicate both cometbft-db and cometbft by forcing the use of cgo and docker:
    • rocksdb / grocksdb
    • cleveldb
  • eliminates the need for build tags

@faddat faddat requested a review from a team as a code owner February 15, 2024 10:05
Copy link

codecov bot commented Feb 15, 2024

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

@@            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 Coverage Δ
pebble.go 72.64% <100.00%> (-0.92%) ⬇️
badger_db.go 89.03% <33.33%> (+1.25%) ⬆️

... and 11 files with indirect coverage changes

Files Coverage Δ
pebble.go 72.64% <100.00%> (-0.92%) ⬇️
badger_db.go 89.03% <33.33%> (+1.25%) ⬆️

... and 11 files with indirect coverage changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved
@faddat faddat changed the title bump golang and pebble remove Dockerfile, use only goleveldb and pebble Feb 15, 2024
@faddat
Copy link
Contributor Author

faddat commented Feb 15, 2024

@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
Copy link
Contributor Author

faddat commented Feb 15, 2024

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

@faddat
Copy link
Contributor Author

faddat commented Mar 14, 2024

I think I just fixed that annoying codeql thing.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/govulncheck.yml Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
goleveldb.go Outdated Show resolved Hide resolved
pebble.go Outdated Show resolved Hide resolved
pebble.go Outdated Show resolved Hide resolved
pebble.go Outdated Show resolved Hide resolved
pebble.go Outdated Show resolved Hide resolved
@melekes
Copy link
Contributor

melekes commented Mar 18, 2024

eliminates unmaintained database backends with no future:
bolt
badger

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

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@faddat
Copy link
Contributor Author

faddat commented Mar 18, 2024

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

@faddat
Copy link
Contributor Author

faddat commented Mar 23, 2024

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 faddat changed the title remove Dockerfile, use only goleveldb and pebble remove Dockerfile, use only pure go dabase backends Mar 23, 2024
@faddat
Copy link
Contributor Author

faddat commented Mar 29, 2024

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

@adizere
Copy link
Member

adizere commented Apr 3, 2024

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:

  • golang to v1.22.* and pebble v1.1.0
  • mark cleveldb and bold as deprecated
  • Docker improvements

@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
Copy link
Contributor Author

faddat commented Apr 4, 2024

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants