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!: remove cleveldb and boltdb #2786

Merged
merged 2 commits into from
Apr 12, 2024
Merged

feat!: remove cleveldb and boltdb #2786

merged 2 commits into from
Apr 12, 2024

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Apr 11, 2024

cleveldb and boltdb were deprecated in v1.


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@melekes melekes requested review from a team as code owners April 11, 2024 14:02
@melekes melekes self-assigned this Apr 11, 2024
@andynog andynog added P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably storage labels Apr 11, 2024
Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

lgtm, added a few comments nothing major

Comment on lines 10 to +12
key-value database. If you want maximal performance, it may be best to install
the real C-implementation of LevelDB and compile CometBFT to use that using
`make build COMETBFT_BUILD_OPTIONS=cleveldb`. See the [install
RocksDB and compile CometBFT to use that using
`make build COMETBFT_BUILD_OPTIONS=rocksdb`. See the [install
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this suggestion based on any evidence that RocksDB provides the maximal performance ? I understand in the past we suggested cleveldb because of the C-Implementation (not sure if that was also an accurate statement), but maybe we should re-word the maximal performance to something like one of the best performance or something like this. I don't think we have any benchmarks to support this statement (afaik we didn't test one against the other and we even included Pebble recently that might provide good performance too).

Copy link
Contributor

Choose a reason for hiding this comment

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

@melekes , @andynog , just seeing this. I wonder whether Andy is right and we might want to suggest people to use either rocksdb or pebble. Or we could run the benchmarks on rocksdb to be sure of this. We could also link to the storage report and report the fact that pebble does good compaction vs goleveldb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmalicevic what you're suggesting sounds good to me. The changes above to the documentation were more like a quick patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw that this text was actually removed with a subsequent PR. I can open a small issue to extend this with the one line saying there are multiple options and to look for these two as well for better performance. But its low priority now that we are not explicitly suggesting rocksdb.

@@ -80,46 +80,46 @@ git pull origin main
make install
```

## Compile with CLevelDB support
## Compile with RocksDB support
Copy link
Contributor

Choose a reason for hiding this comment

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

were these commands tested on both Ubuntu and MacOS ? Or just a rename ? I am assuming these new commands and instructions would work fine.

test/e2e/generator/generate.go Show resolved Hide resolved
test/e2e/pkg/manifest.go Show resolved Hide resolved
test/e2e/pkg/testnet.go Show resolved Hide resolved
@andynog andynog added this pull request to the merge queue Apr 12, 2024
Merged via the queue into main with commit 81d8d07 Apr 12, 2024
39 checks passed
@andynog andynog deleted the remove-boltdb-cleveldb branch April 12, 2024 12:44
melekes added a commit that referenced this pull request Apr 15, 2024
until we have settled on a single database - #1039. Note I still think
RocksDB is faster even thought I don't have data to back it up. Anyway,
I don't think we should "endorse" any database in the meantime.

Refs #2786 (comment)
github-merge-queue bot pushed a commit that referenced this pull request Apr 15, 2024
until we have settled on a single database - #1039. Note I still think
RocksDB is faster even thought I don't have data to back it up. Anyway,
I don't think we should "endorse" any database in the meantime.

Refs
#2786 (comment)
melekes added a commit that referenced this pull request Aug 12, 2024
@melekes melekes mentioned this pull request Aug 12, 2024
4 tasks
github-merge-queue bot pushed a commit that referenced this pull request Aug 13, 2024
follow-up to #2786

---

#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Andy Nogueira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably storage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants